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 redundant branches in generated switch body, fixes #6671 #6672

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

friedbyalice
Copy link
Contributor

@friedbyalice friedbyalice commented Mar 12, 2024

Hi, I'm new to this project and honestly to ocaml, let me know if the fix seems reasonable and what would be the most appropriate way to propose it (if direct PR is not your style).

I'm looking for forward for comments on this possible approach to solve the issue, the general idea is that the issue is caused by loss of information when translating from typed tree to labmda tree, therefore the easiest way to avoid the issue is to implement an optimization. That said I know that the optimization could be implemented in other places, and at the same time I hope that it's sound.

@zth
Copy link
Collaborator

zth commented Mar 12, 2024

Nice job! cc @cristianoc who's best to review this.

Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for the fix.
Would you update the changelog too?

@zth should this go in v11 or master?

@zth
Copy link
Collaborator

zth commented Mar 13, 2024

I think v11. @enzo-pellegrini could you rebase to 11_release, and add a changelog?

@friedbyalice friedbyalice changed the base branch from master to 11.0_release March 13, 2024 09:20
@zth zth enabled auto-merge (squash) March 13, 2024 09:51
@zth
Copy link
Collaborator

zth commented Mar 13, 2024

@enzo-pellegrini thank you for a superb initial contribution!

@zth zth merged commit 9263e4f into rescript-lang:11.0_release Mar 13, 2024
14 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.

3 participants