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

Pattern matching refactoring #1560

Merged
merged 3 commits into from Jan 9, 2018

Conversation

Projects
None yet
2 participants
@gasche
Copy link
Member

gasche commented Jan 5, 2018

These are minor refactoring suggested by the ongoing work on parmatch.ml. I create separate PRs for those changes for ease of review.

cc @trefis

The first commit reverts a non-standard "code-blocking" (is that a word?) style that @trefis used in #1550, with ( ) instead of begin end for multi-line blocks -- I have no strong opinion on which is better but grepping for else ($ and else begin$ shows that typing/ uses the latter.

The second commit changes full_match to return false when given an empty list (which is semantically correct) instead of asserting false as it previously did. This allows to factorize some cases where empty and non-empty constrs can be handled uniformly. This point came up while writing #1552, and I will rebase #1552 if the current PR is accepted.

@trefis
Copy link
Contributor

trefis left a comment

The second commit changes full_match to return false when given an empty list (which is semantically correct) instead of asserting false as it previously did.

I'm confident that works / won't cause a problem in practice.

As for the rest of the changes, I find the "style consistency" argument fairly ridiculous considering the current general state of the codebase (although I'm not saying you're not right in this particular instance about the usage of begin-end compared to parentheses under typing/).
But since, contrarily to what you said, you were apparently offended enough by the use of parentheses over begin-end then please be my guest and change it.

However, as we're now into stylistic discussion I would ask that you please do not encourage the multiplication of:

bla bla bla match whatever with
| ... -> ...

where clearly everyone should be writing

bla bla bla
  match whatever with
  | ... -> ...

or at the very least:

bla bla bla match whatever with
  | ... -> ...
*)

let full_match closing env = match env with
| ({pat_desc = (Tpat_any|Tpat_var _|Tpat_alias _|Tpat_or _)},_) :: _ ->
assert false (* we assume we are working with simplified patterns *)

This comment has been minimized.

@trefis

trefis Jan 6, 2018

Contributor

We assume more than that.
You're allowed to assume the absence of variables, aliases and or-patterns on simplified matrices, but Tpat_any can still be present.
Here we can safely rule it out because we're called on the result on the constrs of build_specialized_submatrices and omega is not a discriminating constructor.

This comment has been minimized.

@gasche

gasche Jan 6, 2018

Author Member

Good point, I will fix the comment, thanks.

Yesterday I was vaguely considering the idea of having private types for sub-types of patterns (simplified patterns and heads/discriminants), but that could be a change for another day.

This comment has been minimized.

@trefis

trefis Jan 6, 2018

Contributor

I think I mentioned when we met a few months ago, but this summer I started a branch doing this. The thing is such a change also impacts matching (unless I messed something up, which is a possibility) and I at the time lacked the familiarity with matching to progress any further.
I still plan to revive that branch but yes, another day.

match p.pat_desc with
if full_match false constrs
then for_constrs ()
else begin match p.pat_desc with
| Tpat_construct _ ->

This comment has been minimized.

@trefis

trefis Jan 6, 2018

Contributor

Please, no.

Also, I only reindented these two lines, I didn't format them. So "reverting a non-standard style that trefis used" my ass. :)

This comment has been minimized.

@gasche

gasche Jan 6, 2018

Author Member

I will change back -- personally I think that not having enough delimiters is worse than playing tricks with the indentation level, so the new version is better than the previous one.

This comment has been minimized.

@trefis

trefis Jan 6, 2018

Contributor

Ah! But here is the good news: you can have both! Delimiters and indentation.

This comment has been minimized.

@gasche

gasche Jan 6, 2018

Author Member

Well in fact it's possible to keep begin match and revert the rest, so I'm doing that.

@gasche gasche force-pushed the gasche:pattern-matching branch from 8cfff5c to a84ef1f Jan 6, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Jan 6, 2018

I changed and rebased the patchset according to @trefis comments.

@gasche gasche force-pushed the gasche:pattern-matching branch 2 times, most recently from 5949b19 to 40ca506 Jan 9, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Jan 9, 2018

@trefis: I just rebased this on top of #1552's merging. Would you consider approving if you still think it is good?

@trefis

trefis approved these changes Jan 9, 2018

Copy link
Contributor

trefis left a comment

Looks good to me.

@gasche gasche merged commit ea0b3cf into ocaml:trunk Jan 9, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gasche gasche referenced this pull request Jul 24, 2018

Closed

Cleanup parmatch #3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.