Skip to content

Replace a misuse prone internal API with a better one.#565

Merged
ltratt merged 1 commit into
softdevteam:masterfrom
ratmice:vacant_marking
May 16, 2025
Merged

Replace a misuse prone internal API with a better one.#565
ltratt merged 1 commit into
softdevteam:masterfrom
ratmice:vacant_marking

Conversation

@ratmice
Copy link
Copy Markdown
Collaborator

@ratmice ratmice commented May 16, 2025

occupied_entry was documented as not being useful for insert, but ok for setting various marks. This is because OccupiedEntry combines insertion with lookup by returning the previous value. In doing so it assumes that the previous value exists, where in the case of the occupied_entry function called from a VacantEntry there was no previous value.

Instead this patch just adds the various marking functions to VacantEntry avoiding the problematic assumptions.

I didn't add mark_used, because it seems out of place and sequence, marking as used is typically done late after lookup, not early before insertion. Oddly the tests do mark_used early, but only because they needed an extra mark to cover all the internal states of the VacantEntry field pos I did add set_merge_behavior even though it isn't used, because at least with that I can think of uses for it.

`occupied_entry` was documented as not being useful for `insert`,
but ok for setting various marks. This is because `OccupiedEntry`
combines insertion with lookup by returning the previous value.
In doing so it assumes that the previous value exists, where in
the case of the `occupied_entry` function called from a
`VacantEntry` there was no previous value.

Instead this patch just adds the various marking functions
to `VacantEntry` avoiding the problematic assumptions.
@ltratt ltratt added this pull request to the merge queue May 16, 2025
Merged via the queue into softdevteam:master with commit a883a57 May 16, 2025
2 checks passed
@ratmice ratmice deleted the vacant_marking branch May 16, 2025 19:26
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