Skip to content

[matching.ml cleanup] introductory changes#8768

Merged
trefis merged 55 commits intoocaml:trunkfrom
trefis:matching-1st-pr
Jul 30, 2019
Merged

[matching.ml cleanup] introductory changes#8768
trefis merged 55 commits intoocaml:trunkfrom
trefis:matching-1st-pr

Conversation

@trefis
Copy link
Copy Markdown
Contributor

@trefis trefis commented Jun 25, 2019

First wave of changes, joint work with @gasche (with help from @maranget).
I could add a changelog entry in this PR already, but I have a slight preference for delaying this until all the changes (more PRs are in preparation) have been submitted.

The commits here are focused on documenting, renaming, and dusting off some old code. The code emitted by the compiler should be strictly the same before and after this PR (we have checked that this is true on the compiler codebase).

trefis and others added 24 commits June 4, 2019 13:06
Enabled only on lambda/matching.ml
And commit the config we decided to use for that file.
The assert didn't trigger, the tests passed even when returning v2.
@gasche
Copy link
Copy Markdown
Member

gasche commented Jun 25, 2019

My script to check that the generated code is the same within the compiler distribution is the following:

make clean
./configure
make core -j
make coreboot -j
make partialclean
sed -i 's/COMPFLAGS=-strict-sequence/COMPFLAGS=-dlambda -dno-unique-ids -dump-into-file -strict-sequence/g' Makefile
make world -j
make opt -j
git checkout Makefile
mkdir -p lambda-dumps
for f in $(find . -name '*.dump')
do
    cp --parents $f lambda-dumps
    rm $f
done
make clean

More precisely, this will create a directory lambda-dumps with the -dlambda representation of all the OCaml files compiled during make world opt. I do this one on the base branch, once from the PR branch, and diff the directories to check the changes. (Obviously the files that are changed by the patch will change; but the others won't, which suggests that the pattern-matching compiler did not change behaviors in obvious ways.)

(The way -dlambda prints locations on everything makes it painful/difficult to compare two slightly different versions of a file. Eventually I would like to submit a PR to have -dno-unique-ids also hide locations in -dfoo printing.)

in
List.iter (fun ((_, lbl, _) as x) -> t.(lbl.lbl_pos) <- x) lbls;
Array.to_list t
| _ -> fatal_error "Parmatch.all_record_args"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Matching.all_record_args rather than Parmatch. ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed. This has been here for two years now, but let's take the opportunity to change it.

However or-patterns are only half-simplified,
- aliases under or-patterns are kept
- or-patterns whose left-hand-side is simplifiable
are removed: (_|p) is changed into _
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here, the code feels less specific than the comment since any or-pattern p|q where p ≤ q is simplified to p. Maybe we could add a for instance in the comment, or write down the effective rule. Or maybe,

  • or-patterns whose right-hand-side is subsumed by their left-hand side are simplified to their left-hand-side:
    For instance [( :: | 1 :: _ )] is changed into [ _ :: _ ]

| [] -> raise (Invalid_argument "cut")
| a :: l ->
let l1, l2 = cut (n - 1) l in
(a :: l1, l2)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Except for the Invalid_argument (and the behavior on negative integers), isn't this cut function the same as rev_split_at?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looks like it is indeed.

@Armael
Copy link
Copy Markdown
Member

Armael commented Jul 23, 2019

I started reading this PR a few weeks ago, and I won't have time to finish, so here are the few remarks I had in the meantime.
NB: my reviewing process was to try to check commit by commit that the new code is equivalent to the old code. I did not try to actually understand what it is doing, so I cannot say anything about the new explanatory comments.

I read until commit 181f2a7 (i.e. I haven't read the last 5 commits).

  • commit 18bdcd0, I couldn't see why the new code is equivalent to the old code. Maybe I missed something.
  • d4af329 then 8357206, tests pass indeed, but I don't know why the alternative implementation is correct. Someone else with actual insight should check.
  • I noted that the code seems to assume that may_compat is a symmetric operation, check and document that it is the case
  • b7f1589 what happened to the case if Or_matrix.safe_below_or_matrix rev_ors (p, ps) then?
  • 17b0213 does that mean that precompile_or now has ors <> [] as pre-condition? If yes, add an assert to make that explicit

@trefis trefis force-pushed the matching-1st-pr branch from 584ed5c to eb829ac Compare July 29, 2019 12:56
Apparently ocamlformat isn't quite reliable in that area.
@trefis trefis merged commit 3ead1bd into ocaml:trunk Jul 30, 2019
trefis added a commit to trefis/ocaml that referenced this pull request Aug 2, 2019
[matching.ml cleanup] introductory changes
trefis added a commit to trefis/ocaml that referenced this pull request Aug 2, 2019
[matching.ml cleanup] introductory changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants