Conversation
trefis
left a comment
There was a problem hiding this 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
| ... -> ...
typing/parmatch.ml
Outdated
|
|
||
| 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 *) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if full_match false constrs | ||
| then for_constrs () | ||
| else begin match p.pat_desc with | ||
| | Tpat_construct _ -> |
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah! But here is the good news: you can have both! Delimiters and indentation.
There was a problem hiding this comment.
Well in fact it's possible to keep begin match and revert the rest, so I'm doing that.
8cfff5c to
a84ef1f
Compare
|
I changed and rebased the patchset according to @trefis comments. |
5949b19 to
40ca506
Compare
* Adds guard type to Parsetree * Resolve merge conflicts * resolves more merge conflicts * adds pattern guard typechecking * fix formatting * fix test output * adds back deleted new line Co-authored-by: Nick Roberts <nroberts02@gmail.com> * semicolon at end of record Co-authored-by: Nick Roberts <nroberts02@gmail.com> * `in` on new line Co-authored-by: Nick Roberts <nroberts02@gmail.com> * adds exhaustiveness warnings to pattern guards * bind guard pattern variables in cmt2annot * print typed pattern guard in-line * refactor rec_check for match expressions * documentation and better naming in typecore * documents type_match * clean up type_cases * add documentation for Typedtree guard type * update ocamlprof to use new pattern guard datatype * new tests * moved comment about constructor in variant * further typecore cleanup * changed type and parameter names to use `partial` * sequence and `let`...`in` formatting Co-authored-by: Nick Roberts <nroberts02@gmail.com> * move match to new line Co-authored-by: Nick Roberts <nroberts02@gmail.com> * update typedtree pattern guard documentation Co-authored-by: Nick Roberts <nroberts02@gmail.com> * break lines in typedtree guard pattern docs * CR for typedtree guard constructor rename * replaced optional bool arg with "warn_if" * type-directed disambiguation and fix test output * update warning name * updates `type_match` documentation * format: add newline for match Co-authored-by: Nick Roberts <nroberts02@gmail.com> * format: remove spaces between record item and semicolon Co-authored-by: Nick Roberts <nroberts02@gmail.com> * CR-soon adding partial to typedtree * format: semicolon after final record field Co-authored-by: Nick Roberts <nroberts02@gmail.com> * check exception pattern in guard for expansiveness * CR-soon name change for [Guard_pattern] * fixed exception pattern check * fix name of warning in tests * add more descriptive warning description for pattern guard total match Co-authored-by: Nick Roberts <nroberts02@gmail.com> * update `match_info` documentation Co-authored-by: Nick Roberts <nroberts02@gmail.com> * format: move short cases first * add line breaks to accepted GH suggestions * format: newline before `in` Co-authored-by: Nick Roberts <nroberts02@gmail.com> * added pattern guard judgement documentation * correct judgement order for patterns * update name of `check_match` in uses * refactors guard pattern expansiveness check * further refactored to avoid creating a function * fixed indentation in expansiveness check * fix default expansiveness --------- Co-authored-by: Nick Roberts <nroberts02@gmail.com>
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 ofbegin endfor multi-line blocks -- I have no strong opinion on which is better but grepping forelse ($andelse begin$shows thattyping/uses the latter.The second commit changes
full_matchto returnfalsewhen 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-emptyconstrscan be handled uniformly. This point came up while writing #1552, and I will rebase #1552 if the current PR is accepted.