Remove enum string feature #7858
Conversation
Pull Request Test Coverage Report for Build jorgen.edelbo_335Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build jorgen.edelbo_336Details
💛 - Coveralls |
| size_t index = size_t(static_cast<Array*>(m_arr)->get(ndx)); | ||
| return m_string_enum_values->get(index); | ||
| } | ||
| case Type::interned_strings: { |
There was a problem hiding this comment.
It has not been deleted, but the diff view plays tricks on you. It is the next function that is deleted.
| @@ -1002,7 +990,6 @@ void setup_multi_table(Table& table, size_t rows, std::vector<ObjKey>& keys, std | |||
| auto string_col = table.add_column(type_String, "string"); // 4 | |||
| auto string_long_col = table.add_column(type_String, "string_long"); // 5 | |||
| auto string_big_col = table.add_column(type_String, "string_big_blobs"); // 6 | |||
| auto string_enum_col = table.add_column(type_String, "string_enum"); // 7 - becomes StringEnumColumn | |||
There was a problem hiding this comment.
Shouldn't the numbers in the comment being decreased as well?
There was a problem hiding this comment.
I think the numbers dates back to the days where we referenced columns by their index and not by the stable ColKey. But in a sense it is ok to leave the numbers as the ColKeys will not change.
| // Create the enumkeys list if needed | ||
| if (!m_enumkeys.is_attached()) { | ||
| m_enumkeys.create(Array::type_HasRefs, false, m_num_public_columns); | ||
| m_top.set(4, m_enumkeys.get_ref()); |
There was a problem hiding this comment.
what should we do with Spec::m_enumkeys and Spec::s_enum_keys_ndx ? I think they can be removed as nobody should be using them, but maybe we should also assert that there is nothing stored there?
There was a problem hiding this comment.
That was an oversight. Should be removed.
| int_fast64_t v(from_ref(mem.get_ref())); | ||
| spec_set.add(v); // Throws | ||
| dg_2.release(); | ||
| spec_set.add(from_ref(mem.get_ref())); // Throws |
There was a problem hiding this comment.
the old code seems to be very cautious about allocation failure...
are we safe without it because we can rely on commit rollbacks? Alternatively, could we order things like this?
spec_set.add(0);
MemRef mem = create_empty_array(...);
spec_set.set(spec_set.size() - 1, from_ref(mem.get_ref()));
There was a problem hiding this comment.
I think all these destroy guards are from the time where we could have freestanding tables. They are not needed when we have transactions. Allocations will always be done in the slab area and it will be reset by every commit/rollback (reset_free_space_tracking()).
There was a problem hiding this comment.
Great, thanks for confirming!
| @@ -115,50 +99,35 @@ MemRef Spec::create_empty_spec(Allocator& alloc) | |||
| // The 'spec_set' contains the specification (types and names) of | |||
| // all columns and sub-tables | |||
| Array spec_set(alloc); | |||
| _impl::DeepArrayDestroyGuard dg(&spec_set); | |||
src/realm/spec.hpp
Outdated
| @@ -100,7 +100,7 @@ class Spec { | |||
| ArrayStringShort m_names; // 2nd slot in m_top | |||
| Array m_attr; // 3rd slot in m_top | |||
| // 4th slot in m_top not cached | |||
| Array m_enumkeys; // 5th slot in m_top | |||
| // 5th slot in m_top | |||
There was a problem hiding this comment.
I'd suggest adding to the comment something like this: "5th slot in m_top, old enum keys which was never released"
What, How & Why?
This feature is no longer relevant as string interning will now be performed automatically.
☑️ ToDos
bindgen/spec.yml, if public C++ API changed