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

Support tuple patterns #232

Merged
merged 25 commits into from
Nov 15, 2024
Merged

Support tuple patterns #232

merged 25 commits into from
Nov 15, 2024

Conversation

chengluyu
Copy link
Member

No description provided.

@chengluyu chengluyu force-pushed the hkmc2-ucs2 branch 3 times, most recently from 9f0ccc9 to eaf60b3 Compare November 11, 2024 00:44
@chengluyu chengluyu force-pushed the hkmc2-ucs2 branch 4 times, most recently from 00faff0 to 251d1c1 Compare November 13, 2024 07:53
@chengluyu chengluyu marked this pull request as ready for review November 13, 2024 07:54
Copy link
Contributor

@LPTK LPTK left a comment

Choose a reason for hiding this comment

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

Excellent, thanks! I only have two main change requests.

First, please add a test that pattern-matches tuples within a function fun foo(tupleGet) = .... Realize your foolishness. Then fix the bug.

Second, please comment out the vast majority of the uses of the :ucs flag. This produces extremely verbose debugging output that's mostly noise and creates so many diff changes. Especially since it prints symbol UIDs (it probably shouldn't). At most, please only keep a few small but representative uses of :ucs, which can be used to check if there is a regression in the UCS logic. Currently there is just so much output noise that any change creates tons of diffs and regressions may as well go unnoticed, which completely defeats the purpose of diff-testing.

hkmc2/shared/src/main/scala/hkmc2/semantics/Symbol.scala Outdated Show resolved Hide resolved
hkmc2/shared/src/test/mlscript/ucs/syntax/TupleRest.mls Outdated Show resolved Hide resolved
hkmc2/shared/src/test/mlscript/codegen/BadIdent.mls Outdated Show resolved Hide resolved
Copy link
Contributor

@LPTK LPTK left a comment

Choose a reason for hiding this comment

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

Awesome contribution, thanks!

@LPTK LPTK merged commit de16b05 into hkust-taco:hkmc2 Nov 15, 2024
1 check passed
@LPTK LPTK deleted the hkmc2-ucs2 branch November 15, 2024 01:23
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