Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

easy::Value equality doesn't handle keys being in other orders #194

Closed
epage opened this issue Sep 10, 2021 · 2 comments · Fixed by #196
Closed

easy::Value equality doesn't handle keys being in other orders #194

epage opened this issue Sep 10, 2021 · 2 comments · Fixed by #196
Labels
C-bug Category: Things not working as expected

Comments

@epage
Copy link
Member

epage commented Sep 10, 2021

Noticed this when porting toml-rs tests. linked_hash_map requires two maps to have keys in the same order to be equal.

Options

  • easy::Map could have its own equality
  • We can switch to indexmap, which is what toml-rs uses
@epage epage added the C-bug Category: Things not working as expected label Sep 10, 2021
@ordian
Copy link
Member

ordian commented Sep 11, 2021

It loses ordering on remove but I suspect that is less of a problem with us tracking table positions. We don't track inline table positions

If we only delete by replacing the value with Item::None, then it's even less of a problem. I was considering replacing linked_hash_map with indexmap as well esp. considering it's only in maintenance mode: https://github.com/contain-rs/linked-hash-map. I think we should do it.

@epage
Copy link
Member Author

epage commented Sep 11, 2021

Good idea with the tombstones!

EDIT: Looks like they aren't needed, there is shift_remove that preserves order

epage added a commit to epage/toml_edit that referenced this issue Sep 11, 2021
@epage
fix: Proper equality for easy::Value
c9348c2 
`LinkedHashMap`s equality checked order when we don't want it to.  It
also isn't maintained.  So we're switching to `IndexMap`

Unfortunately, there are other ordering issues in the relevant test that
makes it hard to get right, so went ahead and removed it.

Fixes toml-rs#194
epage added a commit to epage/toml_edit that referenced this issue Sep 11, 2021
@epage
fix: Proper equality for easy::Value
4fa5630 
`LinkedHashMap`s equality checked order when we don't want it to.  It
also isn't maintained.  So we're switching to `IndexMap`

Unfortunately, there are other ordering issues in the relevant test that
makes it hard to get right, so went ahead and removed it.

Unfortunately, I didn't see any change in performance.

Fixes toml-rs#194
@epage epage mentioned this issue Sep 11, 2021
Merged
epage added a commit that referenced this issue Sep 11, 2021
@epage
fix: Proper equality for easy::Value (#196)
2ff172e 
`LinkedHashMap`s equality checked order when we don't want it to.  It
also isn't maintained.  So we're switching to `IndexMap`

Unfortunately, there are other ordering issues in the relevant test that
makes it hard to get right, so went ahead and removed it.

Unfortunately, I didn't see any change in performance.

Fixes #194
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-bug Category: Things not working as expected
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Proper equality for easy::Value epage/toml_edit
2 participants
@epage @ordian