Assert AR table entry stability on replace #13705
Conversation
hash.c
Outdated
| ar_table_pair *orig_pair = pair; | ||
| (*replace)(&key, &val, arg, TRUE); | ||
|
|
||
| // TODO: pair should be same as pair before. | ||
| pair = RHASH_AR_TABLE_REF(hash, i); | ||
| HASH_ASSERT(orig_pair == pair); |
There was a problem hiding this comment.
Reading this, I think what the original TODO may have intended is that in non-debug builds, maybe we don't need to do the RHASH_AR_TABLE_REF lookup a second time? What do you think?
| ar_table_pair *orig_pair = pair; | |
| (*replace)(&key, &val, arg, TRUE); | |
| // TODO: pair should be same as pair before. | |
| pair = RHASH_AR_TABLE_REF(hash, i); | |
| HASH_ASSERT(orig_pair == pair); | |
| (*replace)(&key, &val, arg, TRUE); | |
| // Pair should not have moved | |
| HASH_ASSERT(pair == RHASH_AR_TABLE_REF(hash, i)); |
There was a problem hiding this comment.
This makes sense to me. I tried committing your patch but got the error:
Applying suggestions on deleted lines is currently not supported.
I'm going to manually apply your patch and push it for review.
I'd love for @ko1 to comment on his original intent for this TODO.a
|
@jhawthorn Anything else you need me to change before this is merged? |
|
Nope! I meant to merge this last week. |
|
It's nice to see you opening PRs to Ruby Erik ❤️ |
|
Thanks! Now that they're being merged, I'm encouraged to make some more! |
Improve hash internal iteration by verifying that AR table entries remain stable after
st_update_callback_funcreplaces a key-value pair. Fixes a TODO by @ko1 from 2019.