Skip to content

Pattern matching refactoring (also)#1561

Closed
gasche wants to merge 3 commits intoocaml:trunkfrom
gasche:pattern-matching-2
Closed

Pattern matching refactoring (also)#1561
gasche wants to merge 3 commits intoocaml:trunkfrom
gasche:pattern-matching-2

Conversation

@gasche
Copy link
Copy Markdown
Member

@gasche gasche commented Jan 5, 2018

cc @trefis

This change was suggested by @trefis during the review of #1552.

Sometimes we may want to refer to two different simplify_first_col
functions, if you manipulate two kind of rows at once -- typically,
negative and positive rows need not carry the same metadata.

@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jan 5, 2018

This change is best reviewed while ignoring whitespace:

https://github.com/ocaml/ocaml/pull/1561/files?w=1

Copy link
Copy Markdown
Contributor

@trefis trefis left a comment

Choose a reason for hiding this comment

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

Thanks.

Let's wait for the other PR to be merged before taking any action?

Changes Outdated
### Internal/compiler-libs changes:

- GPR#1488: Refreshing parmatch
- GPR#1488, GPR#1561: Refreshing parmatch
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wouldn't add that here.
I would add it to your other GPR which started changing the names. (so yes, I would wait for the other to be merged and then rebase this PR, if I were you)

*)

let rec every_satisfiables pss qs = match qs.active with
let rec every_satisfiables pss qs = let open Usefulness in match qs.active with
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:/

Copy link
Copy Markdown
Member Author

@gasche gasche Jan 6, 2018

Choose a reason for hiding this comment

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

I generally try to avoid changing the indentation level from a patch, but here we can change it if we review with whitespace ignored anyway.

@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jan 6, 2018

Let's wait for the other PR to be merged before taking any action?

My idea (in making a separate PR) was rather that this one could be merged relatively quickly (that is, if we agree), whereas #1552 was awaiting an extra review for correctness. So I planned to rebase #1552 on top of the present one. But if you prefer to wait, well we can always wait :-)

@gasche gasche force-pushed the pattern-matching-2 branch from df246fc to 4f7aab1 Compare January 6, 2018 11:53
@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jan 6, 2018

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

@gasche gasche force-pushed the pattern-matching-2 branch 2 times, most recently from c3f8e7a to c79b5e7 Compare January 9, 2018 17:41
@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jan 9, 2018

@trefis: I just rebased this on top of #1552, but I also added a new commit, would you maybe have a look -- and consider approving?

@gasche gasche force-pushed the pattern-matching-2 branch from 25e2a42 to 25e5794 Compare January 9, 2018 21:44
gasche added 3 commits January 9, 2018 22:49
The need for this change arose while working on ambigous variables:
sometimes you may want to refer to two different `simplify_first_col`
functions, if you manipulate two kind of rows at once -- typically,
negative and positive rows need not carry the same metadata.
@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jan 10, 2018

(I also had to rebase on top of #1560 which introduced minor conflicts)

@trefis
Copy link
Copy Markdown
Contributor

trefis commented Jan 16, 2018

I finally had a look and discussed this offline with gasche and the conclusion is that we're going to merge some version of the second patch (i.e. the refactoring making simplify_head_pat more generic) but hold off doing the first set of changes until a later date as we're not completely satisfied with the proposed API.

@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jan 19, 2018

Closing this in favour of #1577.

@gasche gasche closed this Jan 19, 2018
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.

2 participants