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

Shadow the polymorphic comparison in the middle-end #1811

Merged

Conversation

Projects
None yet
8 participants
@xclerc
Copy link
Contributor

commented Jun 1, 2018

Almost everything is in the title...

This PR introduces a module, namely Int_replace_polymorphic_compare,
that defines the compare function and the comparison operators to work
on int values. The module is then opened at the top of every module in
the middle-end. This guarantees that the polymorphic comparison cannot
be used by mistake (this PR being a follow-up of #1810).

(There were suspicious comparisons in Flambda_utils.sameswitch, where =
was used to compare values of type Numbers.Int.Set.t.)

@bluddy

This comment has been minimized.

Copy link

commented Jun 1, 2018

Polymorhphic comparison strikes again!

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2018

@chambart I am looking for a reviewer; would you be interested?

@@ -193,7 +190,7 @@ and same_named (named1 : Flambda.named) (named2 : Flambda.named) =
| Move_within_set_of_closures _, _ | _, Move_within_set_of_closures _ ->
false
| Prim (p1, al1, _), Prim (p2, al2, _) ->
p1 = p2 && Misc.Stdlib.List.equal Variable.equal al1 al2
Stdlib.(=) p1 p2 && Misc.Stdlib.List.equal Variable.equal al1 al2

This comment has been minimized.

Copy link
@kit-ty-kate

kit-ty-kate Jun 18, 2018

Contributor

What is the type of p1 and p2 here ?

This comment has been minimized.

Copy link
@xclerc

xclerc Jun 18, 2018

Author Contributor

The type is Lambda.primitive, which has more than 100 constructors.
I reckon something should be done here; either write an equality function
for Lambda.primitive (and the types it references), or at the very least
ensure that the type does not change over time. However, one can
genuinely be worried that one day a float value might pop up in the
definition of Lambda.primitive (or one of its numerous referenced types).

This comment has been minimized.

Copy link
@xclerc

xclerc Jun 18, 2018

Author Contributor

(For the record, and the impatient reader, this is the only place
where I kept the polymorphic comparison.)

This comment has been minimized.

Copy link
@yallop

yallop Jun 18, 2018

Member

Perhaps it'd be worth adding a Lambda.primitive_equal function (initially an alias for Stdlib.(=)) to keep the definition of equality close to the definition of the type.

This comment has been minimized.

Copy link
@xclerc

xclerc Jun 18, 2018

Author Contributor

@yallop Sure; if memory serves I put all introduced equality functions
in the same modules as the type definitions.

This comment has been minimized.

Copy link
@kit-ty-kate

kit-ty-kate Jun 18, 2018

Contributor

Maybe it might be worth adding a comment to Lambda.equal_primitive to know why it uses polymorphic equality.

This comment has been minimized.

Copy link
@xclerc

xclerc Jun 18, 2018

Author Contributor

Indeed; should I say that I am too lazy to write the equality
function because there are too many constructors and nested
types involved?

This comment has been minimized.

Copy link
@kit-ty-kate

kit-ty-kate Jun 18, 2018

Contributor

Sure, it's a totally valid comment ^^

xclerc added some commits Jun 18, 2018

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2018

There are some lines over 80 characters. #1148 will be merged soon, which will turn that into a CI error:

./bytecomp/lambda.mli:217.81: [long-line] line is over 80 columns
./middle_end/flambda_utils.ml:81.81: [long-line] line is over 80 columns
./middle_end/inlining_cost.ml:484.81: [long-line] line is over 80 columns
./middle_end/inlining_transforms.ml:113.81: [long-line] line is over 80 columns
./middle_end/simplify_boxed_integer_ops.ml:73.81: [long-line] line is over 80 columns
./middle_end/simplify_boxed_integer_ops.ml:74.81: [long-line] line is over 80 columns
./middle_end/simplify_boxed_integer_ops.ml:88.81: [long-line] line is over 80 columns
./middle_end/simplify_primitives.ml:227.81: [long-line] line is over 80 columns
./middle_end/simplify_primitives.ml:228.81: [long-line] line is over 80 columns
./middle_end/simplify_primitives.ml:229.81: [long-line] line is over 80 columns
@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2018

@dra27 the reported issues should be fixed by 6595154.

By the way, I am still looking for a reviewer; @chambart maybe?

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2018

@xclerc - it does indeed, thanks. The failure on Travis is a subtle bug in the CI script, which I just fixed in f395dfc, so the failure can either be ignored or should disappear on any future push to this branch.

@chambart chambart self-requested a review Jul 3, 2018

@chambart
Copy link
Contributor

left a comment

This looks good to me, and fixes some problems.

let is_not_none = function
| None -> false
| Some _ -> true
in

This comment has been minimized.

Copy link
@chambart

chambart Jul 3, 2018

Contributor

This may go into Misc.Option

@chambart

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2018

I'm happy to merge when the check-typo test will pass

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2018

check-typo failed originally because of a bug in the script (it read the files from an older commit, so the failure was already fixed on trunk). Now it's being weird again as I restart it!

However: this needs a Changes entry and pushing that should hopefully kick the check-typo job. If it doesn't, I'll take it from here 🙂

xclerc added some commits Jul 3, 2018

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2018

Everything is green - ready to merge!

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

@xclerc alas GitHub says you need to rebase and there will be conflicts.

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2018

@damiendoligez I am slightly confused: the github interface says
"This branch has no conflicts with the base branch".

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

Well I'm confused too: what GitHub tells me is:

This branch cannot be rebased due to conflicts

screen shot 2018-07-09 at 16 21 08

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2018

@damiendoligez - it may be that it can be merged, but it can’t be rebased (that would make sense, as trunk has been merged into this branch)

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

@damiendoligez So, can this PR be merged on your end?

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

@dra27 is right, I can squash-and-merge or do a merge commit, but I cannot rebase-and-merge, most likely because you merged trunk instead of the usual (rebasing the PR on top of trunk).

So, @xclerc do you think the commit history is useful (in which case I should merge) or should I just squash everything into one commit?

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2018

@damiendoligez I don't think the history is useful here; you can squash
everything into one commit. Thanks!

@damiendoligez damiendoligez merged commit cd06e22 into ocaml:trunk Jul 11, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@damiendoligez

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

Thanks!

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

I just saw a build failure on the "parallel build" CI machine whose log (below) suggests that it might be related to a missing make depend step after this PR is merged.

File ".../workspace/parallel-build/flambda/false/label/ocaml-linux-64/_none_", line 1:
Error (warning 58): no cmx file was found in path for module Int_replace_polymorphic_compare, and its interface was not compiled with -opaque
Makefile:1293: recipe for target 'middle_end/debuginfo.cmx' failed
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.