Skip to content

Erase unboxed products #105

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

Merged
merged 4 commits into from
Mar 4, 2025
Merged

Conversation

ccasin
Copy link

@ccasin ccasin commented Feb 25, 2025

This adds erasure for unboxed tuples and records. It's simple - we just parse them as the boxed versions. Somewhat arbitrarily ask @tdelvecchio-jsc for review, since @dvulakh is already on the hook for a different PR. The first commit does tuples and the second records

How can you review the tests?

  1. Open these files and check that the only #s are unrelated to unboxed products.
test/passing/tests/unboxed_tuples-erased.ml.ref
test/passing/tests/unboxed_tuples_cmts_attrs-erased.ml.ref
test/passing/tests/unboxed_record-erased.ml.ref
test/passing/tests/unboxed_record_cmts_attrs-erased.ml.ref
  1. Run these commands and observe in each case that the diff is all about #:
patdiff test/passing/tests/unboxed_tuples{.ml,-erased.ml.ref}
patdiff test/passing/tests/unboxed_tuples_cmts_attrs{.ml,-erased.ml.ref}
patdiff test/passing/tests/unboxed_record{.ml,-erased.ml.ref}
patdiff test/passing/tests/unboxed_records_cmts_attrs{.ml,-erased.ml.ref}

Copy link

@tdelvecchio-jsc tdelvecchio-jsc left a comment

Choose a reason for hiding this comment

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

For the -erased tests, could you remove the .js-ref files and replace them with .why-no-js?

How come we don't have to make any changes to the standard normalization?

Shall I do tests on our internal code?

@ccasin
Copy link
Author

ccasin commented Mar 2, 2025

@tdelvecchio-jsc thanks for the review

For the -erased tests, could you remove the .js-ref files and replace them with .why-no-js?

Done

How come we don't have to make any changes to the standard normalization?

I figured the tests would catch me if I needed to update any normalization logic. But on reflection I think that's wrong, and I'm just getting lucky that these files don't also fail the extended check. So I've added this logic.

Shall I do tests on our internal code?

Since this doesn't change formatting, I'm not sure this is needed, but I'll defer to your judgment.

Copy link

@tdelvecchio-jsc tdelvecchio-jsc left a comment

Choose a reason for hiding this comment

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

May do a bit more thorough testing in the future, but for now this seems clearly correct.

@ccasin ccasin merged commit f69f637 into janestreet:jane Mar 4, 2025
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants