matching: simplify can_group #9417
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Octachron
reviewed
Apr 3, 2020
Octachron
approved these changes
Apr 6, 2020
review of can_group factorization by @gasche: > I reviewed the change to `can_group` and believe it is correct. > (I knew it would be nicer as a binary operation!) > > The different treatments of Lazy and Tuple/Record does look a bit odd, > but I believe that it is actually the right thing to write. > > In the Tuple or Record case, the idea is that we can group Any heads > with the Tuple/Record case and just explode all of them (including the > Any cases) according to the tuple/record arity. > > In the Lazy case, the corresponding choice would be to add Any values > to the Lazy group, and force all the elements of the group. This is > not so nice, because intuitively we shouldn't force Lazy values unless > we have to. > > One may expect that in general the argument of the pattern will be > forced anyway, as long as there is at least one Lazy pattern above in > the matrix, so it doesn't really matter whether we include the Any > patterns in the forced group or not. I would argue that (1) even if > that was true, it would be semantically dubious to handle Any that way > (see a more precise criterion below), (2) I am not actually sure that > this is true, what if the first group gets found unreachable by > splits in following columns? > > # type void = | ;; > # match (lazy (print_endline "foo"), (None : void option)) with > | lazy (), Some _ -> . > | _, None -> false;; > - : bool = false > > This gives the following decision tree for whether Any is included in > the group: > > - Can the absence of Any be used to generate nice/efficient tests for > the heads of the group? In that case, don't include Any. > (Not included: all the Constant cases, Array (discriminate on size), > String, etc.) > > - Is Any always exactly equivalent to a more-defined head for values > of this type? In that case, do include Any, otherwise do not. > (Included: Variant, Record) > (Not included: Lazy)
(Report from Florian Angeletti)
gasche
force-pushed
the
rematch-can_group-simplif
branch
from
63f852f
to
d05f86e
Compare
|
(There was a failure in lib-random/random.ml; I rebased and re-pushed so we will see whether it was transient.) |
|
(The failure was transient, merging. @trefis I'll add the missing Changes entry in trunk directly.) |
|
Ack. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note: there are already some comments from a previous review in the commit message.