-
Notifications
You must be signed in to change notification settings - Fork 77
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.
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
core: add Hasher and HashableModule #2210
Conversation
8e39ecf
to
bb66a49
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2210 +/- ##
=======================================
Coverage 89.44% 89.45%
=======================================
Files 319 323 +4
Lines 38684 38741 +57
Branches 5729 5734 +5
=======================================
+ Hits 34601 34655 +54
- Misses 3281 3286 +5
+ Partials 802 800 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that this is quite a convoluted way of implementing a __hash__
.
What would be the downside of computing a key (tuple of operation names) and calling hash
on that? I feel that this might result in much simpler code. (It would also introduce less new code that needs testing)
Furthermore, we should take into account that hashes of mutable objects may result in unexpected/broken behavior, I think having a clearer warning on the HashableModule class would be helpful in communicating the problems associated with that.
I feel like it's pretty standard, it lets users create a hash of complex objects without having all of the things that compose the hash in memory at a time, making it much more efficient than a hash of a tuple of ops. I also don't feel like it's that complicated, I guess that depends on how used to it the reader is. Swift's hash works like this, so I guess I'm used to it. I also didn't want to go just with xor since that drops the order of elements, but I guess few rewrites just reorder operations, so maybe that should be fine for the sake of hashing in the short term. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I wouldn't use just a xor here, as it will have a lot of collisions (to be exact, if you have the same parity of every operation, then it will have the same hash, the order won't matter at all).
But that's something we can always fix later, so I'm fine with merging it now!
I totally agree, we can replace the xor with |
Good idea! |
These utils are planned to be used in `xdsl-gui`, but may well be useful outside of that project. The Hasher contains logic I've used a lot before, it seems to work well enough. In the future, we might want a more efficient way to compute equality/inequality for modules, but we might as well add a working helper class now and optimise later.
These utils are planned to be used in
xdsl-gui
, but may well be useful outside of that project. The Hasher contains logic I've used a lot before, it seems to work well enough. In the future, we might want a more efficient way to compute equality/inequality for modules, but we might as well add a working helper class now and optimise later.