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

Fix longjmp marshaling tests, clean up relifting #1023

Merged
merged 14 commits into from
Mar 29, 2023
Merged

Conversation

sim642
Copy link
Member

@sim642 sim642 commented Mar 28, 2023

Besides just fixing the necessary, this does broader cleanup with the hope to avoid similar issues again.

Changes

  1. Remove identity Printable.Std.relift which hides missing definitions.
  2. Add all missing relift definitions. Some rather core types like MapDomain were not relifting their inner components.
  3. Introduce Printable.StdLeaf as Printable.Std alternative with identity relift. This is only to be used with primitive and CIL types which don't contain inner types that could require relifting.
  4. Remove default Printable.Std.name, which names everything "std". This makes debugging such issues a total pain, e.g. "lifted std and either std or std option" is completely ineligible. All printables must specify a name explicitly.
  5. Add all missing name definitions.

TODO

  • Readd relifts for all other marshaling tests.

@sim642 sim642 added the bug label Mar 28, 2023
@sim642 sim642 self-assigned this Mar 28, 2023
@sim642 sim642 added the cleanup Refactoring, clean-up label Mar 29, 2023
@sim642 sim642 changed the title Fix longjmp marshaling tests Fix longjmp marshaling tests, clean up relifting Mar 29, 2023
@sim642 sim642 marked this pull request as ready for review March 29, 2023 08:39
@michael-schwarz michael-schwarz self-requested a review March 29, 2023 09:11
@michael-schwarz
Copy link
Member

Might be worth doing some of the incremental benchmarks here, if this is very expensive, it might be worth taking steps to avoid relifting where it is not needed due to the current configuration.

@michael-schwarz
Copy link
Member

Also we might think about writing a deriver for relift? For most cases I don't see a reason we cannot do it.

@sim642
Copy link
Member Author

sim642 commented Mar 29, 2023

Might be worth doing some of the incremental benchmarks here, if this is very expensive, it might be worth taking steps to avoid relifting where it is not needed due to the current configuration.

That could be rather error-prone again though. If all possibilities for sub-printables are not accounted for in a particular configuration, it leads to similar hard-to-debug issues.
Recursing more might cause us to do extra work, but overall this would be linear in the size of the data. Traversing that in memory shouldn't be slower than reading it from a file.

Also we might think about writing a deriver for relift? For most cases I don't see a reason we cannot do it.

I'll add it to #31. I started working on it at some point, but it's been in hibernation for a long time. I wanted a more general and maintainable approach than writing a dozen ppx derivers which duplicate large parts of the code just for a different function name/return type/number of arguments/etc.

@sim642 sim642 merged commit f46ffc6 into master Mar 29, 2023
@sim642 sim642 deleted the longjmp-relift branch March 29, 2023 11:26
@sim642 sim642 added this to the v2.2.0 milestone Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cleanup Refactoring, clean-up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants