[matching.ml cleanup] enable the extra division in more cases#8869
[matching.ml cleanup] enable the extra division in more cases#8869trefis merged 2 commits intoocaml:trunkfrom
Conversation
lambda/matching.ml
Outdated
| | Tpat_var _ -> | ||
| true | ||
| | Tpat_alias (p, _, _) -> heads_are_var p | ||
| | Tpat_or (p1, p2, _) -> heads_are_var p1 && heads_are_var p2 |
There was a problem hiding this comment.
Is it necessary to check p2 here? After all, when simplifying Tpat_or, we replace ω | … by ω directly. It seems that we could implicitly apply this rule here and only have heads_are_var p1.
There was a problem hiding this comment.
There was a problem hiding this comment.
I don't think it is wise to change the code based on what we know simplification is going to do in the future -- it makes the code harder to understand by adding a dependency on invariants from some other part of the code (and: we discussed disabling this simplification anyway, assuming that it is uncommon as it fires the "unused subpattern" warning). The logic of that particular function is that it is a specialized instance of a generic "collect all the potential heads of this pattern" followed by an implicit List.for_all traversal; in general the heads of p | q are the heads of p and the heads of q, and following this general pattern gives a clear structure to the code.
There was a problem hiding this comment.
After more in-person grumbling from @Octachron we agreed to change the definition to use ... p1 || ... p2, and I renamed the function (jamais deux sans trois) into omega_like.
dc9fbb8 to
5c5b98c
Compare
One optimization in split_no_or is the insertion of a split before a last matrix line that has only variables. Splitting a matrix there creates two default environments (instead of one for the non-split matrix), the first of which often gets specialized away by further refinement, and the second one jumping directly to the catch-all case -- this produces better code. The code would detect this case (all-variable last row) by calling `group_var` on all the patterns of the row, but the `group_*` functions assume that their input is already simplified, and only the first column of the row is simplified. The present commit fixes it by defining a predicates that does not assume the pattern is simple, and check that "all its heads" are a variable. In the future we will refactor this code using Parmatch.Simple_head.t, and the function invocation may change. Note: two testcases are changed in tests/basic/patmatch_split_no_or, the first is the actual optimization and the second is just stamp-related noise.
|
I'm OK with the new name, not convinced by the change to |
5c5b98c to
b08c4e2
Compare
|
The transformation is correct whether we use |
I agree with that.
I assume you mean "it's not important enough that I care about it anymore". |
|
And yes, whatever, just put |
Octachron
left a comment
There was a problem hiding this comment.
After "grumbling" a bit, I agree that the change is correct (and that the Tpat_or case doesn't really matter (so we can afford to not transform "or"s into "and"s)).
|
Thanks @Octachron! I'll merge once appveyor finishes whatever it's doing. |
The commit message explains what's going on.
Apart from that, I also add the comments I told @Octachron I would add in #8861, but was prevented from doing because of @gasche's eagerness to click merge. :)