Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More lambda/matching documentation #12442

Merged
merged 7 commits into from Aug 3, 2023

Conversation

gasche
Copy link
Member

@gasche gasche commented Jul 28, 2023

@trefis and myself met today to work on #7241 together, and our first order of business was to read, document and refactor when useful some parts of the code that remained obscure to us in the past -- and had become obscure to @maranget when we discussed this a couple months ago.

This PR contains no behavior change, only documentation comments and some minor refactorings.

@Octachron has done a first round of in-person review.

Copy link
Contributor

@lthls lthls left a comment

Choose a reason for hiding this comment

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

I've looked at all the commits, and I think they're safe, and the comments do help a little bit.

I have some doubts about the commit that switches the order of elements in a list (ddb9fad): the list is not used as an association list in any obvious way, and the comment still states that the list is ordered without specifying which order (if I understand correctly, it only means that order has meaning in this list, and that it's not just a dictionary). So I think switching to a record would have made sense, but switching the order of the tuples doesn't seem particularly useful.
The rest of the comments are not enough for me to understand how the algorithm works, but they don't feel misleading so I'd rather have them than not.

gasche and others added 7 commits August 3, 2023 21:07
Co-authored-by: Luc Maranget <Luc.Maranget@inria.fr>
Co-authored-by: Thomas Refis <thomas.refis@gmail.com>
- remove dead comment
- next_matchs => next_matches
- total_* => jumps_*
- li => lambda_i
- avoid redundant matching on non-empty list 'rem'
  ((_, x) :: xs) becomes ((_, second_match) :: next_next_matches)

Co-authored-by: Thomas Refis <thomas.refis@gmail.com>
Co-authored-by: Thomas Refis <thomas.refis@gmail.com>
Co-authored-by: Thomas Refis <thomas.refis@gmail.com>
Co-authored-by: Thomas Refis <thomas.refis@gmail.com>
@gasche gasche force-pushed the more-matching-documentation branch from a4dea45 to 69e1730 Compare August 3, 2023 19:08
@gasche
Copy link
Member Author

gasche commented Aug 3, 2023

the list is not used as an association list in any obvious way

Correct: a pair (i, matrix) represents an exit-handling clause of the form

| exit i -> <compiled matrix>

but we never List.assoc into this list. We still believe that it is expected to have the exit number before the exit handler code. (This is also more consistent with the other exit-indexed structure in the code, namely jump summaries -- Jumps.t.)

the comment still states that the list is ordered without specifying which order (if I understand correctly, it only means that order has meaning in this list, and that it's not just a dictionary)

Yes, the ordering that matters is the source order -- not preserving it would break the top-down priority order of matching clauses.

@gasche gasche merged commit 70ec240 into ocaml:trunk Aug 3, 2023
8 of 9 checks passed
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.

None yet

2 participants