Skip to content

Add an introductory comment at the top of the file#9

Closed
trefis wants to merge 13 commits intorematch-pattern-head-408from
matching-comment
Closed

Add an introductory comment at the top of the file#9
trefis wants to merge 13 commits intorematch-pattern-head-408from
matching-comment

Conversation

@trefis
Copy link
Copy Markdown
Owner

@trefis trefis commented Jul 2, 2019

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?

@trefis
Copy link
Copy Markdown
Owner Author

trefis commented Jul 2, 2019

Ah, and, yes : ping @gasche .

@trefis
Copy link
Copy Markdown
Owner Author

trefis commented Jul 3, 2019

I have pushed fixes for all your remarks.

@trefis
Copy link
Copy Markdown
Owner Author

trefis commented Jul 10, 2019

And I added some more comments on split_no_or and precompile_or (that last one being reaaaaally meh)

catch exit_num ->
<this pm's body>
v}
and keep going, otherwise we drop it and proceed to the next one.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a commit doing that reordering.

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
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how you'd do that, using ocamldoc's reference syntax?
Why don't you push that change on this branch?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed something along these lines.

| ( C1(_, argO)
| C2(argO) ) , p3 , q3 -> lam3
| C3 , p4 , q4 -> lam4
| C1(_, _) , _ , _ -> lam5
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember, are patterns really split into a head and the arguments (pushed into the row) at this point?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are in cls yes.

@trefis trefis force-pushed the matching-3rd-pr-408 branch from af29fb0 to 952d50c Compare July 15, 2019 16:32
@trefis trefis force-pushed the matching-comment branch from b81af77 to 94e0a00 Compare July 15, 2019 16:42
@trefis trefis changed the base branch from matching-3rd-pr-408 to rematch-pattern-head-408 July 15, 2019 16:43
Copy link
Copy Markdown
Collaborator

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:"?

@trefis trefis closed this Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants