pattern-matching refactoring: merge the two matchers#9493
pattern-matching refactoring: merge the two matchers#9493gasche merged 10 commits intoocaml:trunkfrom
Conversation
|
@Octachron remarks that, for pattern heads, in many cases we only use the |
|
Note: reconstructing a pattern from a head is a more natural implementation in Parmatch (rebuilding counter-examples, etc.) and probably done less often in Matching. |
c0625a2 to
39042a8
Compare
|
I just rebased on top of trunk. There were conflicts due to the sprinkling of |
|
I just pushed a last commit that fine-tunes some aspects of |
a47015a to
8b8fb62
Compare
Octachron
left a comment
There was a problem hiding this comment.
This PR merges together the two matcher functions by making them more regular, without any behavior change. The invariant that records need to be expanded is a bit inelegant, but no more than the pre-existing code and it could be fixed later.
Overall, this is a clear and nice code simplification.
This commit is delicate and needs a careful review. The `matcher_of_pattern` function is a temporary measure to reduce the invasiveness of the patch, and make it easier to review. (Note for reviewers: in the previous version the Record case had a funny handling of Any, but it is in fact equivalent to just adding omegas as we now do in all cases.) There are two obvious directions for improvement: - Get rid of matcher_of_pattern and pass a head directly to the various make_matching_* functions. - Try to factorize this code with ctx_matcher which, it is now obvious, does essentially the same thing. Another, less immediate area of attack would be to consider a presentation of Pattern_head.t where the Any case can be statically ruled out -- maybe the description could have two levels, one isomorphic to option (Any or not?) and one for non-any heads.
f1f47a9 to
1c14957
Compare
1c14957 to
3903f9f
Compare
|
I addressed the remaining comments of @Octachron, this should be good to merge if/when the CI returns. |
|
Merged! Thanks a lot @Octachron for the review, which is also produced nice suggestions for things we have to work on in this refactoring. |
This is the next bit of the big pattern-matching refactoring change (#9321) after #9464 has been merged.
(cc @trefis @Octachron)
It is a series of refactoring with two highlights:
matcher_*functions to a singlematcheron a head. This is similar in spirit to the move from thegroup_*functions to a singlecan_groupfunction on a head in acd44f9 ( matching: simplify can_group #9417 ).ctx_matcherfunction from Context (for optimization), which deduplicates the matching logic.As previously, this is best reviewed commit-by-commit.