refactor: drop ElementId; identity = position in tree#15
Merged
Conversation
Eliminates the ElementId field on Counter / Register / Map. Identity for recursive merge comes from the slot's (key, kind) tuple in the containing Map. Removes the elementid module entirely and the dedicated death test binary that exercised the cross-arena composite abort. - map_merge's recursive guard simplifies to (same kind, both alive) — no id comparison. - map_merge's LWW composite path now element_clones the winner into the dst arena instead of host_abort'ing. Cross-arena composite displacement is supported and observable. - map_set tolerates type-flip at a key via LWW (composite displaces scalar, scalar displaces composite, different-kind composite displaces). - New get-or-create helpers: map_counter / map_register / map_map. Each derives the composite at the given key (same kind → return existing pointer; different kind / empty / tombstone → install fresh if stamp wins LWW; stamp loses → return detached composite). - Deep-copy clones land for all primitives: scalar_clone (renamed from scalar_dup), register_clone, counter_clone, map_clone, and the element_clone dispatcher. - map_clone skips element_clone on tombstones — stale value field would otherwise read as undefined; covered by a new regression that probes arena_used after cloning a deleted big-subtree slot. - map_clone now checks hashtable_insert; was unchecked. - counter_inc / counter_dec factored through a counter_entry_for helper removing ~50 lines of duplication. - scalar_eq's string branch uses memcmp instead of a hand-rolled loop. Drops: elementid.c/h, test_elementid.c, test_map_abort.c, counter_id / register_id / map_id accessors, ElementId arg from all *_create.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 21 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…rings - map_merge: skip element_clone + map_set when src would lose LWW against an existing dst entry. The clone is a deep recursive copy into dst's arena, and map_set would discard the write anyway — so the work was guaranteed-unreachable when src lost. - scalar_eq: short-circuit zero-length string comparison before memcmp. memcmp on NULL pointers is UB pre-C2x even with size 0; the original byte loop was safe here, my prior refactor was not.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
ElementIdfield onCounter/Register/Map. Identity for recursive merge =(key, kind)in the containing Map.elementid.c/h,test_elementid.c, andtest_map_abort.c(death test for the now-defunct cross-arena composite abort).*_createno longer takeElementId;counter_id/register_id/map_idaccessors gone.map_mergerecursive guard simplified to(both alive, same kind, kind != SCALAR).map_mergeLWW composite path nowelement_clones the winner into the dst arena instead ofhost_aborting.map_settolerates type-flip at a key via LWW (composite displaces scalar, scalar displaces composite, different-kind composite displaces).map_counter/map_register/map_map. Same kind in slot → return existing; otherwise install fresh if stamp wins LWW. Stamp loses → return detached composite (Yjs-style: caller always gets a usable handle).scalar_clone(renamed fromscalar_dup),register_clone,counter_clone,map_clone, andelement_clonedispatcher.map_cloneskipselement_cloneon tombstones — stale value field would otherwise read undefined. Regression test probes viaarena_used.map_clonenow checkshashtable_insertreturn.counter_inc/counter_decfactored throughcounter_entry_forhelper. ~50 lines of dup removed.scalar_eq's string branch usesmemcmp.Why
ElementId(PR #14) was a redundant restatement of position in the tree. Every place that consumed it could derive the same answer from(parent path, key). Eliminating it simplifies the merge recursive guard, removes the cross-arena composite abort, unlocks type-flip at a key via LWW + clone, and makes detached-composite construction trivial —counter_create(arena); ...; map_set(m, k, klen, element_counter(c), stamp)just works.273 tests across 10 suites (was 251).
Summary by cubic
Remove ElementId; identity is now the (key, kind) position in the parent Map. This simplifies merges, enables safe LWW type flips, supports cross-arena composites via deep cloning, and avoids wasted clones when a source loses LWW.
Refactors
elementid.*, dropped*_idaccessors, updated*_create(Arena *)APIs.map_mergerecursion to “both alive, same kind”; LWW path clones the winner into dst and now skips clone+set when src would lose.element_clone,scalar_clone(renamed fromscalar_dup),register_clone,counter_clone,map_clone;map_cloneskips tombstones and checkshashtable_insert.counter_entry_for;scalar_equsesmemcmpwith a zero-length guard.New Features
map_counter/map_register/map_map(return existing if kind matches; otherwise install if stamp wins; losing stamp returns a detached composite handle).Written for commit 92ea0e9. Summary will update on new commits.