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

Parmatch: introduce a type for simplified pattern heads #8766

Merged
merged 4 commits into from Jul 16, 2019

Conversation

trefis
Copy link
Contributor

@trefis trefis commented Jun 24, 2019

This is a mostly mechanical change, that has been a long time coming.

I think @lpw25 is a good candidate to review this, given that he too had wanted to implement this (or a similar) idea for a long time.

typing/parmatch.ml Outdated Show resolved Hide resolved
typing/parmatch.ml Outdated Show resolved Hide resolved
Copy link
Contributor

@stedolan stedolan left a comment

Choose a reason for hiding this comment

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

Looks good! Glad to see a bunch of assert false go away.

typing/parmatch.ml Outdated Show resolved Hide resolved
typing/parmatch.ml Show resolved Hide resolved
typing/parmatch.ml Show resolved Hide resolved
Copy link
Member

@Octachron Octachron left a comment

Choose a reason for hiding this comment

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

I like the code. Nevertheless, I have some small minor nitpicking remarks. In particular, the code often uses names of the form d... for pattern head variables. Could we use more mnemonic names? Like h.. as a shorthand for head (which is also used) for instance?

typing/parmatch.ml Outdated Show resolved Hide resolved
; typ = Ctype.none
; env = Env.empty
; attributes = []
}
Copy link
Member

Choose a reason for hiding this comment

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

Minor nitpicking: why not use make at this point?

typing/parmatch.ml Show resolved Hide resolved
typing/parmatch.ml Show resolved Hide resolved
typing/parmatch.ml Show resolved Hide resolved
typing/parmatch.ml Outdated Show resolved Hide resolved
@lpw25
Copy link
Contributor

lpw25 commented Jul 15, 2019

Note from a brief discussion with @trefis. In a future PR it might be worth adding a version of Pattern_head.destruct that returned a list of "destructed" patterns, allowing it to work on or-patterns. It would allow for simpler code in places like compat and simplify_head_pat.

@gasche
Copy link
Member

gasche commented Jul 15, 2019

I have thought about extending the notion of head to or-patterns (which may have several heads). One property that we lose as soon as we leave the fragment of simple patterns is the equivalence between pattern_desc and head * pattern list. Indeed, this equivalence does not take into account the binding structure "above" the head of the pattern, as present in the cases Tpat_var and Tpat_alias.

So I think that rather than extending destruct, we should have a different API for this case that emphasize that we are talking about the shape of the matched values, as a lossy projection from the pattern: heads : pattern -> head list.

@trefis
Copy link
Contributor Author

trefis commented Jul 16, 2019

@Octachron I updated as you suggested. I'll clean up the history and merge later today.

@gasche:

So I think that rather than extending destruct, we should have a different API for this case

I believe that is what @lpw25 was suggesting.

@trefis
Copy link
Contributor Author

trefis commented Jul 16, 2019

Ah, I realize I forgot to answer:

In particular, the code often uses names of the form d... for pattern head variables. Could we use more mnemonic names? Like h.. as a shorthand for head (which is also used) for instance?

d... is often used to indicate we're working with a "discriminating" pattern (which I guess we should call "discriminating head") as produced by discr_pat, not just any head.
I will have another look at the code to see if it's actually used properly.

@trefis
Copy link
Contributor Author

trefis commented Jul 16, 2019

I pushed some minor renaming along these lines, but things were mostly OK IMO.

@Octachron
Copy link
Member

The renamings look nice (in particular in simple_match_args), and make it much more clearer that d is a shorthand for discriminating, thanks!

@trefis
Copy link
Contributor Author

trefis commented Jul 16, 2019

I rebased and cleaned up the history.
I'll merge once the CI is green.

@trefis trefis merged commit b598cb0 into ocaml:trunk Jul 16, 2019
@trefis trefis deleted the pattern_head branch July 16, 2019 12:48
trefis added a commit to trefis/ocaml that referenced this pull request Aug 2, 2019
Parmatch: introduce a type for simplified pattern heads
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

5 participants