Skip to content

Fix to_prev_sibling_byte losing the child of a list node location - #111

Merged
luketpeterson merged 3 commits into
Adam-Vandervorst:masterfrom
imlvts:fix/prev-sibling-child-slot
Sep 19, 2026
Merged

luketpeterson merged 3 commits into
Adam-Vandervorst:masterfrom
imlvts:fix/prev-sibling-child-slot

Conversation

@imlvts

@imlvts imlvts commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

A location with both a value and a child is two same-key slots in a LineListNode. The previous-sibling lookup only checked the first matched slot for the child, so the zipper landed with child_count 0. Check both.

A location with both a value and a child is two same-key slots in a
LineListNode. The previous-sibling lookup only checked the first matched
slot for the child, so the zipper landed with child_count 0. Check both.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019R2H8fnco29asY2v3TPbtF
@imlvts

imlvts commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator Author

https://github.com/Adam-Vandervorst/PathMap/actions/runs/35158141349#summary-105008105822

Again, +57.2% superdense_drop_bench/200, but overall result not changed?

…st macro that is implemented for all zipper types; slightly broadening covered cases
@luketpeterson

Copy link
Copy Markdown
Collaborator

@imlvts I promoted your test to all zippers and added another test. That turned up an ACTZipper bug.

I committed a fix for that bug, but I wanted your review in case the fix isn't agreeable. I factored it the way I did because having the focus at a non-existent path is an edge case in practice and I didn't want the case to slow down anything that is doing iteration over existing items.

@luketpeterson

Copy link
Copy Markdown
Collaborator

Again, +57.2% superdense_drop_bench/200, but overall result not changed?

I feel like the drop bench results are so noisy, I'm of a mind to remove those benchmarks altogether.

If I thought this was the final shape of the TrieNode contract, I'd optimize the function more, but I don't think it's worth any time right now because there are a number of changes I want to make.

We don't really have any benchmarks that exercise backwards iteration, but maybe we should.

@imlvts

imlvts commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator Author

This conflicts with #113, and I will update that later. The way you did it is better.
I'll look into benchmarks, my understanding is that it's only act value count that behaves weird.
If you're happy, we can merge this.

@luketpeterson
luketpeterson merged commit 07382ac into Adam-Vandervorst:master Sep 19, 2026
4 checks passed
@luketpeterson

Copy link
Copy Markdown
Collaborator

Opened #120 to track the benchmark discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants