[matching.ml cleanup] structured rows#9322
Conversation
There were some patches on the file since our last series of patch that didn't respect the formatting. So we're calling ocamlformat again (the same version as before, something < 0.10) so everything rebases cleanly.
|
(This is the first batch of changes of #9321, split because it makes sense as a unit to review on its own.) |
d8fdcd4 to
c1d7714
Compare
Octachron
left a comment
There was a problem hiding this comment.
I like the general clarification, and I am pretty confident that there is no change in behaviour (I need one or more pass of review to be sure). However, I have few questions on interfaces, names and comments:
|
|
||
| type clause = pattern Non_empty_clause.t | ||
|
|
||
| val try_no_or : Half_simple.pattern -> pattern option |
There was a problem hiding this comment.
I found myself backtracking a few time on the name try_no_or. Maybe the function name could reflect what it is trying to do : Something like promote_no_or?
There was a problem hiding this comment.
The rationale was to have assert_foo functions and try_foo functions, the first which does a cast or fail and the other which returns an option. assert_foo does not exist here, and try_foo is going to go away with the polymorphic-variant version anyway, so we propose to keep this name temporarily to avoid rebase pains.
lambda/matching.ml
Outdated
| ) | ||
| | _ -> | ||
| (* I really want to assert false here. *) | ||
| { args; cells = [] } |
There was a problem hiding this comment.
I would prefer an assert false too.
| pretty_precompiled_res next nexts | ||
| ); | ||
| (next, nexts) | ||
|
|
There was a problem hiding this comment.
The debug part in split_and_precompile_* is really noisy. I think it should be factorized, otherwise the small difference between the three variants are lost in the noise.
There was a problem hiding this comment.
@Octachron remarked that this is duplication we introduced in this PR, so it would be better fixed now. I added two more commits to do it.
| and compile_simplified repr partial ctx (m : Simple.clause pattern_matching) = | ||
| match m with | ||
| | { cases = []; args = [] } -> comp_exit ctx m | ||
| | { args = ((Lvar v as arg), str) :: argl } -> |
There was a problem hiding this comment.
The local precondition args = ((Lvar v as arg), str) :: argl is new. But compile_simplified is only called in do_compile_matching which is only called by compile_match and compile_half_compiled that respect this precondition.
| (m : pattern Non_empty_clause.t pattern_matching) = | ||
| match m with | ||
| | { cases = []; args = [] } -> comp_exit ctx m | ||
| | { args = ((Lvar v as arg), str) :: argl } -> |
There was a problem hiding this comment.
This also a new precondition, but compile_half_compiled is only called by compile_flattened, which is called by do_for_multiple_match on a argument list that respect this precondition too. (I have a series of commit at https://github.com/Octachron/ocaml/commits/argo_analysis that makes this point explicit).
There was a problem hiding this comment.
I have a series of commit at https://github.com/Octachron/ocaml/commits/argo_analysis that makes this point explicit
Thanks, but we're not going to integrate that right now.
Let's revisit once all the other PRs have been reviewed.
lambda/matching.ml
Outdated
| ~mk_action:mk_new_action ~vars:(List.map fst vars) rem_cases | ||
| in | ||
| let handler = | ||
| { provenance = [ [ orp (* FIXME? *) ] ]; |
There was a problem hiding this comment.
🤷♂️
The handler seems to be the same as before; and we can't remember why we added a FIXME. So we'll remove it.
c1d7714 to
05b55b2
Compare
This triggers if the first column contains something other than a variant (this is similar to the "get_key"s for other heads).
05b55b2 to
f3e6fc7
Compare
Octachron
left a comment
There was a problem hiding this comment.
The current state looks good to me: it is helpful to have the different stages of the pattern compilation reflected in the types.
Here we stop representing pattern rows as a list of patterns and instead switch to a pair of head pattern (previously the first element of the list) and the rest of the row (previously the tail of the list).
We also start using the
Pattern_head.ttype (fromParmatch) in places.