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.

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

Use ppx_deriving_hash 0.1.2 #1395

Merged
merged 4 commits into from
Mar 26, 2024
Merged

Use ppx_deriving_hash 0.1.2 #1395

merged 4 commits into from
Mar 26, 2024

Conversation

sim642
Copy link
Member

@sim642 sim642 commented Mar 20, 2024

This gets rid of some ppx_deriving_hash related TODOs by using a version of it which includes their support.

@sim642 sim642 added cleanup Refactoring, clean-up setup Dependencies, CI, releasing dependencies Pull requests that update a dependency file labels Mar 20, 2024
@sim642 sim642 added this to the v2.4.0 milestone Mar 20, 2024
@michael-schwarz
Copy link
Member

LGTM!


Side remark: Have you discussed with @stilscher the issues that ppx_deriving_hash breaking the invariants ensured by the Stdlib.hash (hashes being portable between 32 and 64 bit machines) causes for gobview? The search being broken seems to, e.g., be a consequence of this.

@sim642
Copy link
Member Author

sim642 commented Mar 26, 2024

Side remark: Have you discussed with @stilscher the issues that ppx_deriving_hash breaking the invariants ensured by the Stdlib.hash (hashes being portable between 32 and 64 bit machines) causes for gobview? The search being broken seems to, e.g., be a consequence of this.

Briefly, yes. ppx_deriving_hash is hardly the culprit here: Goblint still contains many manual hash implementations that also don't guarantee this. Also hashes from dependencies might not, so it's hard to guarantee that.

I vaguely remember this point coming up before but I don't know where and why. Our Printable.relift exists precisely to fix this by rehashing marshaled data. Maybe the issue at the time was just fixed by properly recursing relift into all the necessary places.

What search is broken right now? During some recent changes I actually tested GobView's syntactic/semantic search and it seemed to work. GobView CI also tests this.

@sim642 sim642 merged commit 496f24a into master Mar 26, 2024
21 checks passed
@sim642 sim642 deleted the ppx_deriving_hash-0.1.2 branch March 26, 2024 15:36
@michael-schwarz
Copy link
Member

GobView's semantic search

Last I heard from Sarah that was still broken.

@sim642
Copy link
Member Author

sim642 commented Mar 26, 2024

Odd, because in #1372 I broke the GobView CI and also fixed it, so it seems to still work there.

@michael-schwarz
Copy link
Member

Ah, nevermind, I wasn't up-to-date it seems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Refactoring, clean-up dependencies Pull requests that update a dependency file setup Dependencies, CI, releasing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants