-
-
Notifications
You must be signed in to change notification settings - Fork 320
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
Pattern match rest as #6030
Pattern match rest as #6030
Conversation
b2c5b0f
to
8caa03b
Compare
c40765b
to
e3174f6
Compare
cd387f5
to
2ef97fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM besides a few comments
element_layout: InLayout<'a>, | ||
elements: &[Pattern<'a>], | ||
opt_rest: &Option<(usize, Option<Symbol>)>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ever (usize, None)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(usize, None)
does happen higher up in the ast if we have a spread that doesn't capture (at least that is my assumption, didn't double check the code the code) (double checked, yeah, that happens and matter higher up the stack).
Originally I had it converting to Option<(usize, Symbol)>
cause mono only cares about it if the symbol exists.
I removed it cause it felt unnecessary to have two checks. One to go to this other format and one in mono to decide if we should generate the the binding for rest. I can change it back to that if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha. Makes sense. No need to change i think
Still need to add some tests, but seems to be working!
Fixes #5187