Add an introductory comment at the top of the file#9
Add an introductory comment at the top of the file#9trefis wants to merge 13 commits intorematch-pattern-head-408from
Conversation
|
Ah, and, yes : ping @gasche . |
|
I have pushed fixes for all your remarks. |
|
And I added some more comments on |
bytecomp/matching.ml
Outdated
| catch exit_num -> | ||
| <this pm's body> | ||
| v} | ||
| and keep going, otherwise we drop it and proceed to the next one. |
There was a problem hiding this comment.
I don't understand this description. In particular, I cannot tell from it if the compiled body are ever duplicated (what if two pms can jump to me, will I appear in two different catch handlers?)
Without looking at the code (it's not clear that the code would be terribly helpful anyway), it looks like the result can be explained with the following structure:
catch
catch
<first body>
with <second exit> ->
<second body>
with <third exit> ->
<third body>
plus the optimization that bodies whose corresponding exit-number is never used are discarded.
There was a problem hiding this comment.
I tried rewriting the explanation in such a way, I'm not terribly fond of the result (I pushed it anyway), I'll let you have a look.
| (combine_constant, combine_construct, combine_array, ...) | ||
| - We recombine the cells using a switch or some ifs, and if the matching can | ||
| fail, introduce a jump to the next pm that could potentially match the | ||
| scrutiny. |
There was a problem hiding this comment.
Wouldn't it be clearer to have the presentation of comp_match_handlers below this one, so that the jumps have been introduced (in the explanation) before you explain how they are used in the explanation? The explanation could be presented in three passes, (1) "precompilation" (into a list of blahs (what's the right name for the pms we get out of precompilation? basic pms? canonical pms? orthogonal pm? simple pms?)), (2) "compilation of each blah", (3) "chaining/linking of the bodies".
(If I understand correctly, the reason why (3) and (2) are interleaved is that if you detect, by compiling the simple pms before you, that you will never be jumped into, then there is no point in compiling yourself. That makes sense, but maybe we can explain this intensional aspect after the main explanation instead of having it influence the presentation structure?)
There was a problem hiding this comment.
I pushed a commit doing that reordering.
bytecomp/matching.ml
Outdated
| and split_no_or cls args def k = | ||
| and split_no_or (cls : Simple.clause list) args def k = | ||
| (* We split the remaining clauses in as few pms as possible while maintaining | ||
| the property stated earlier (cf. comment at the top of the file), i.e. for |
There was a problem hiding this comment.
I would name the comments and refer to the comment by name. (This is more robust to moving content to other files, etc.; and it is how GHC does it.)
There was a problem hiding this comment.
I don't know how you'd do that, using ocamldoc's reference syntax?
Why don't you push that change on this branch?
There was a problem hiding this comment.
I had a low-tech solution in mind, using custom markup syntax that is not recognized by ocamldoc: eg.
Note {pattern compilation overview}: ...
... see the note {compilation overview} ...
I can push this later, if you haven't done it.
There was a problem hiding this comment.
I pushed something along these lines.
| | ( C1(_, argO) | ||
| | C2(argO) ) , p3 , q3 -> lam3 | ||
| | C3 , p4 , q4 -> lam4 | ||
| | C1(_, _) , _ , _ -> lam5 |
There was a problem hiding this comment.
The _, _ part before lam5 creates a risk of confusing some of the _, _ which may be copies of this one or have been inserted by the compilation technique itself. What about using p5, q5 here? (Were you just fed up with the labeling or is there a reason to have catch-alls here?)
There was a problem hiding this comment.
Actually, it's meant to indicate wildcards in the source, and the point of having wildcard in the source of the example is: that way we're sure the row won't be moved above the or-matrix.
I'd be happier with p5, q5, but we would have to add a note saying p5 <= p4 && q5 <= q4 .
| cls = [ ((C1, [ arg11; arg21 ; p1 ; q1]), lam1) | ||
| ; ((C2, [ arg12; p2; q2 ]), lam2) | ||
| ; ((C3, [ p4; q4 ]), lam4) | ||
| ] |
There was a problem hiding this comment.
I don't remember, are patterns really split into a head and the arguments (pushed into the row) at this point?
af29fb0 to
952d50c
Compare
gasche
left a comment
There was a problem hiding this comment.
Let's ship this comment! It's been sitting for too long and we get decreasing returns. (I wanted to come back to read it to reply to some question to the matching.ml review, which means it's time to send it as a PR upstream.)
|
|
||
| (comp_match_handlers) | ||
| Once the pms have been compiled, we stich them back together in the order | ||
| produced by precompilation, resulting in the following structure: |
There was a problem hiding this comment.
There should be a mention that this three-body is just an example (I thought at first of writing the generic case with well-placed ... but this is too ugly). "result in a nested structure like the following three-pm example:"?
I decided to procrastinate.
You should only look at the last commit in this PR (I'm starting to have a lot of branches, it's getting hard to manage :D).
The comment purposefully doesn't go into the details of how defaults and contexts and jump summaries are used. I just wanted to give people a rough idea of the pipeline, without the details.
What do you think?