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

zv: store entries of Dict as BTreeSet to keep it ordered #500

Merged
merged 6 commits into from
Nov 6, 2023
Merged

zv: store entries of Dict as BTreeSet to keep it ordered #500

merged 6 commits into from
Nov 6, 2023

Conversation

mguentner
Copy link
Contributor

adds a test to assert the desired behavior described in #484.

Credits to @sivizius for reviewing and contributing.

closes #484

@zeenix
Copy link
Contributor

zeenix commented Oct 31, 2023

adds a test to assert the desired behavior described in #484.

Cool, thanks for doing this.

Credits to @sivizius for reviewing and contributing.

We don't see the review so will have to take your word for it. :)

zeenix
zeenix requested changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general looking good. Apart from the inline comments in the commits:

  • It looks like @sivizius added 2 commits after to improve your previous commits. They should be squashed back into your commits so we've a nice linear history.
  • Great work on keeping the commits atomic and adding rational to each commit. 👍 Could I also convince you to add emojis as per the recommendation in our contribution guide? :) It just keeps the history very consistent.

zvariant/src/array.rs Outdated Show resolved Hide resolved
zvariant/src/dict.rs Outdated Show resolved Hide resolved
zvariant/src/dict.rs Outdated Show resolved Hide resolved
zvariant/src/dict.rs Outdated Show resolved Hide resolved
zvariant/src/lib.rs Show resolved Hide resolved
zvariant/src/value.rs Outdated Show resolved Hide resolved
zvariant/src/signature.rs Outdated Show resolved Hide resolved
zvariant/src/signature.rs Outdated Show resolved Hide resolved
@mguentner
🧪 zv: add dict_compare test
84ac3e0 
this test is failing
@mguentner mguentner force-pushed the dict_ordered branch 3 times, most recently from 8d65414 to 41882d0 Compare October 31, 2023 18:04
@mguentner
Copy link
Contributor Author

formating issues should be fixed now

@sivizius
Copy link
Contributor

sivizius commented Nov 1, 2023 •
edited
Loading

Credits to @sivizius for reviewing and contributing.

We don't see the review so will have to take your word for it. :)

Here is some context: mguentner@db9474a.

We tried to conform to C-COMMON-TRAITS, so even though impl Hash for Value<'_> is not relevant to solve #484, it was provided for completeness.

BTreeSet was chosen over BTreeMap, so dictionary-entries with the same key but different values are still allowed, which is the current behaviour. We like to avoid any breakage, even though, we are not sure if there is any use case for this.

The custom comparison for Bytes was implemented similar to std::borrow::Cow, because, as mentioned in the commit message, the ownership state of the data should not impact the result of any comparison of the data itself.

There is an open PR mguentner#4 by myself regarding comparison of floating point values using f64::total_cmp, which is probably what we want in case of NaN-values. This methods implements the totalOrder-predicate doi:10.1109/IEEESTD.2008.4610935, section 5.10. Due to the partial_cmp before, only paragraph d is relevant.

zeenix
zeenix approved these changes Nov 1, 2023
View reviewed changes
Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@zeenix
Copy link
Contributor

zeenix commented Nov 1, 2023

There is an open PR mguentner#4

Thanks. I will have a look at it soon. Somehow I didn't get an email notification for that. 🤷

This methods implements the totalOrder-predicate doi:10.1109/IEEESTD.2008.4610935, section 5.10. Due to the partial_cmp before, only paragraph d is relevant.

That link is a 404 for btw. Is it restricted?

@zeenix
Copy link
Contributor

zeenix commented Nov 1, 2023

@mguentner please fix the formatting issue (make sure to squash int the relevant commit) and we should be ready to merge. Thanks so much for this.

@sivizius
Copy link
Contributor

sivizius commented Nov 1, 2023

Thanks. I will have a look at it soon. Somehow I didn't get an email notification for that. 🤷

It is an PR in the repository with the branch of this PR. Maybe that is why?

That link is a 404 for btw. Is it restricted?

Copy-Paste-Fail, I have fixed it.

@zeenix
Copy link
Contributor

zeenix commented Nov 1, 2023

Thanks. I will have a look at it soon. Somehow I didn't get an email notification for that. 🤷

It is an PR in the repository with the branch of this PR. Maybe that is why?

You mean that branch is based on the branch of this PR? I don't think that matters at all. I think there may be an issue with my email alias' forwarding.

That link is a 404 for btw. Is it restricted?

Copy-Paste-Fail, I have fixed it.

Thanks. Happens to us all. :)

@zeenix
Copy link
Contributor

zeenix commented Nov 3, 2023

@mguentner please fix the formatting issue (make sure to squash int the relevant commit) and we should be ready to merge. Thanks so much for this.

@mguentner Please don't give up this close to the finish line. :)

@mguentner
Copy link
Contributor Author

On it. Won't stall it.

@mguentner
Copy link
Contributor Author

@zeenix Should be good to go. Let's see the workflow result 🎰

zeenix
zeenix requested changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise.

zvariant/src/value.rs Outdated Show resolved Hide resolved
mguentner and others added 5 commits November 6, 2023 12:57
@mguentner
✨ zv: implement PartialOrd, Ord for manifestations of Value
bcd4ee7 
BTreeSet / BTreeMap requires the `Ord` trait to be implemented

Co-authored by: Sebastian Walz <[email protected]>
@mguentner
🐛 zv: use a BTreeSet to store entries of Dict
c5b67e3 
this makes the order of `Dict` deterministic and allows
to compare the value two dicts while the order in which
they have been constructed does not matter.

Issue: dbus2#484
@mguentner
✅ zv: correct encoded byte length
a5e64b9 
since Dict.entries is now represented by a BTreeSet instead of a Vec,
the serialized order changes and thus the padding introduced by the
serializer does aswell
@mguentner
✨ zv: implement PartialEq for Bytes manually
71caee0 
allows comparisons of different enum types with same values

Co-authored by: Sebastian Walz <[email protected]>
@sivizius
✨ zv: impl Hash for Value<'_>
2211eec
@zeenix zeenix enabled auto-merge November 6, 2023 13:49
@zeenix zeenix merged commit ce40dfb into dbus2:main Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zeenix zeenix zeenix requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

zvariant: Implement PartialEq for Dict that ignores the order of entries
3 participants
@mguentner @zeenix @sivizius