Matching: document and refactor mk_failaction_pos#12534
Conversation
167f9e7 to
c1b9a6a
Compare
| unioning the specialized contexts of each failure pattern, | ||
| but more efficient -- the union would have a lot of | ||
| redundancy. *) | ||
| let i_fail_pat = list_as_pat i_fail_pats in |
There was a problem hiding this comment.
list_as_pat is not a very informative name, and it is only used here. You could rename it to something like or_pattern_from_list and/or move the definition here.
There was a problem hiding this comment.
I tried to please you and got pulled in a multi-file-change minor refactoring. I propose to send it as a separate PR to be evaluated on its own.
There was a problem hiding this comment.
(The would-be PR is my branch at https://github.com/gasche/ocaml/commits/minor-patterns-refactoring . I am waiting for the present PR to be merged before submitting. )
c1b9a6a to
f4a8f55
Compare
|
I rebased the PR on trunk (this was non-trivial due to the debug printing changes that went in independently). |
f4a8f55 to
1645553
Compare
|
(I have gone through all of @lthls' comments. Thanks!) |
ncik-roberts
left a comment
There was a problem hiding this comment.
I read the old code and new code, and indeed I like the new code structure (and new comment) a lot more.
I've erred on the side on leaving nits about wording, as I'm a relative newcomer to this code and discussion may surface misunderstandings that I have.
| unioning the specialized contexts of each failure pattern, | ||
| but more efficient -- the union would have a lot of | ||
| redundancy. *) | ||
| let i_fail_pat = list_as_pat i_fail_pats in |
|
|
||
| (* In line with the article and simpler than before *) | ||
| (* In [mk_failaction_pos partial seen ctx defs], | ||
| - [partial] is Total if we know the current switch |
There was a problem hiding this comment.
In mk_failaction_neg, we use partial to identify that we can just return None, Jumps.empty and don't bother computing fail patterns. Do you know why we don't do that short-circuiting in mk_failaction_pos as well in the case that fail_pats < match_context_rows? If we trust Total, then it seems that short-circuiting would save us some needless work and keep contexts a bit smaller.
There was a problem hiding this comment.
I don't know. It would also be possible to put this logic in the if sig_complete then check in the caller of mk_failaction_pos (if sig_complete || partial = Total then ...). (Something similar is done in combine_variant.) I would need to ask Luc if he considered this; and probably will. This could lead to a small code improvement in some cases of matching on an instance of a GADT type, where some cases can be eliminated by typing.
lambda/matching.ml
Outdated
| Through its jump summary, [mk_failaction_pos] propagates "negative | ||
| information" about the constructors not taken. For example, if | ||
| a first submatrix contains a match on the [None] constructor, the | ||
| exit point selected for failure will get provided the context | ||
| information that its value must be [Some _]. |
There was a problem hiding this comment.
The last sentence isn't quite accurate. Jumps from other handlers to that exit point might be None. It's more that jumps from this switch to that exit point must be Some _.
There was a problem hiding this comment.
Attempted reformulation:
For example, if a switch only accepts the [None] constructor, [mk_failaction_pos] generates a failure clause along with context information that the value reaching the failure clause must be [Some _].
1645553 to
b05c539
Compare
|
@ncik-roberts thanks! I took your comments into account and rebased the PR. (Hopefully this will encourage a maintainer, possibly @lthls, to approve and merge.) |
|
Merged! Thank you both. |
mk_failaction_posis one of the key operations in compilation of sum constructors. Its current code is fairly opaque -- it is hard to understand independently of the 2001 paper on pattern-matching compilation. I needed to understand the code in the context of fixing #7241, and it is reasonably likely that a future fix would touch this function -- so the fix would be difficult to review if the function is hard to understand.The present PR documents this function and refactors it slightly for readability; it should not change its behavior in any way, and it should be reasonably easy to check this (this is just light, local code movement).
A possible review workflow would be as follows:
mk_failaction_posin trunk and see what you can make sense ofChecking the accuracy of the new documentation (instead of believing it and reviewing the phrasing only) requires more expertise in the pattern-matching compiler, I will try to get @maranget or @trefis to have a look. Other reviewers should not feel responsible for this.