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.ml: refactor `matrix_stable_vars` #1555

Merged
merged 2 commits into from Jan 3, 2018

Conversation

Projects
None yet
2 participants
@gasche
Copy link
Member

gasche commented Jan 3, 2018

cc @trefis (from #1550)

There are no changes related to negative columns in this PR, the plan is to rebase #1552 on top of this one.

parmatch.ml refactoring: introduce an All constructor for `matrix_sta…
…ble_vars`

This simplifies the implementation of the incoherent case.
@trefis

trefis approved these changes Jan 3, 2018

Copy link
Contributor

trefis left a comment

Rereading this, I felt compelled to bikeshed.
But that refactor is fine.

If the first column contains some head constructors, there
is no need to look at stability for the default matrix: all
other submatrices contain the default matrix, so they have
less stable variables. *)

This comment has been minimized.

@trefis

trefis Jan 3, 2018

Contributor

There are two unrelated parts to this comment, personally I'd like it better if there was a visual hint that that's the case (i.e. I'd add a blank line). When there is only one paragraph I have to reach the end to realize that "oh, ok he's talking about something unrelated".

That being said, I don't think the second part of that comment is that useful: the operation (building specialized matrices) is frequent in parmatch, and the doc of build_specialized_submatrices already says what you're repeating here.

This comment has been minimized.

@trefis

trefis Jan 3, 2018

Contributor

Hum, OK:

  • github doesn't display the whole comment on the main interface; when I talk about "the second part" I mean "everything github is showing right now".
  • I had actually missed the last bit of that comment (because of the "oh ok he's just repeating things we already know" mentioned previously).
    Maybe you could shorten the comment to:

    A stable variable must be stable in each submatrix.
    N.B. all the [constrs] submatrices contain the default matrix, so they have less stable variables.

This comment has been minimized.

@gasche

gasche Jan 3, 2018

Author Member

I added a line between the two parts. I would like to keep the long fully-explicit version for now, because the reasoning may change with negative rows and it's convenient to have the full story to think. I think I will have to edit those comments again anyway.

warn about some variables being unstable.
As sad as that might be, the warning can be silenced by splitting the
or-pattern...
*)

This comment has been minimized.

@trefis

trefis Jan 3, 2018

Contributor

In the other places checking coherence, the comment discussing what happens for coherent-but-ill-typed columns is in the else branch, I don't know why you moved it here.

This comment has been minimized.

@gasche

gasche Jan 3, 2018

Author Member

Ok, I moved it back.

@gasche gasche force-pushed the gasche:pattern-matching branch from 669e6b5 to 682f640 Jan 3, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Jan 3, 2018

I made most of the changes suggested by @trefis.

@trefis

This comment has been minimized.

Copy link
Contributor

trefis commented Jan 3, 2018

👍 good to merge.

parmatch.ml: refactor `matrix_stable_vars` by inlining split_rows
The way we split rows is tightly coupled with the semantics of the
stable-variable computation (why can `default` be ignored when
`consts` is non-empty?), so it is actually better to inline the logic
whithin matrix_stable_vars -- now that other factorizations have made
it reasonably concise.

This also makes it easier to reason on currently-in-progress changes
to add negative rows to `matrix_stable_vars`, which affect the
splitting strategy.

@gasche gasche merged commit 4a9ef2f into ocaml:trunk Jan 3, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.