Conversation
|
(cc the usual suspects: @ncik-roberts , @trefis, @lthls, maybe @Octachron and @voodoos if they want to; note that #13121 is also waiting for a reviewer, but reviewing that other PR involves slightly more domain knowledge ) |
There was a problem hiding this comment.
I am not familiar (yet) with the lambda intermediate representation, but given the change only involves "minor refactorings that should obviously not change the behavior", I had a look.
The proposed refactoring does look correct to me and should not impact the behavior of the compilation. Additionally the use of a record instead of an anonymous couple makes the code slightly easier to read.
lambda/matching.ml
Outdated
| type args = (lambda * let_kind) list | ||
| type 'a arg = { | ||
| arg : 'a; | ||
| str : let_kind; |
There was a problem hiding this comment.
Minor nitpick: str is quite a vague field name. It looks like a shortcut for structure, but it might not be so.
There was a problem hiding this comment.
str is the variable name that was previously used in the code in various places, so reusing it makes the diff less noisy thanks to punning
There was a problem hiding this comment.
(but I can also rename the field and then rebase my later PRs with the new field name if that is your preference; which name would you suggest?)
There was a problem hiding this comment.
I think that it is mostly a waste of time, but the reviewer is king. I took 21 minutes to rename str into binding_kind.
This avoids a naming conflict with 'head' as in "pattern head" that I accidentally introduced recently.
378afd0 to
bdf9dd1
Compare
Suggested-by: Florian Angeletti <florian.angeletti@inria.fr>
|
Thanks! Would you by chance be interested in approving-on-behalf for #13121 as well? |
In parallel to #13121, here is another small PR in the review stack to fix The Pattern-Matching Bug.
(If you wonder about the state of this review stack, that is, when this damn thing is going to be fixed or at least you won't get emails about it anymore: I maintain an overview comment that describes the current best-known state. Currently I have 4 PRs to propose after this one, and the first 2 suffice to fix the bug.)
The present PR contains minor refactorings that should obviously not change the behavior of the pattern-matching compiler.
headto refer to "the head argument of an argument list" (once it is in "simple" form), without realizing that it conflicts with the pre-existing usage ofheadto refer to "the head of the pattern" (also called "discriminant", and a pretty important notion in division of matrices). The commit renameshead::restintofirst::restagain, and the typepure_headbecomespure_arg.lambda * let_bindinginto a record with named fields, to prepare for the addition of a third field in an upcoming PR