这是indexloc提供的服务，不要输入任何密码
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.

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
Merged
merged 4 commits into from
Mar 4, 2025
Merged

Erase unboxed products #105

merged 4 commits into from
Mar 4, 2025

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}

mdelvecchio-jsc
mdelvecchio-jsc requested changes Feb 28, 2025
View reviewed changes
Copy link

@mdelvecchio-jsc mdelvecchio-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.

mdelvecchio-jsc
mdelvecchio-jsc approved these changes Mar 3, 2025
View reviewed changes
Copy link

@mdelvecchio-jsc mdelvecchio-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 oxcaml: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

Reviewers

@mdelvecchio-jsc mdelvecchio-jsc mdelvecchio-jsc approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ccasin @mdelvecchio-jsc