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

Ensures CBV semantics when eta-expanding function to eliminates optional arguments #1424

Merged
merged 3 commits into from Oct 18, 2017

Conversation

alainfrisch
Copy link
Contributor

@alainfrisch alainfrisch commented Oct 11, 2017

Fixes MPR#7657.

(Waiting to know whether this goes to 4.06 or not to write the Changes entry.)

@damiendoligez damiendoligez added this to the 4.07-or-later milestone Oct 11, 2017
@mshinwell
Copy link
Contributor

@damiendoligez This surely has to go into 4.06, no?

@mshinwell mshinwell added the bug label Oct 11, 2017
@damiendoligez
Copy link
Member

@mshinwell it's a really obscure bug that was never observed in the wild, not even by the original reporter. If you want to push it for 4.06, we need a review from a typechecker expert. The fix looks innocent enough to my novice eye but I know better than to trust my judgement where typecore.ml is involved.

@lpw25 @garrigue any takers?

@damiendoligez damiendoligez modified the milestones: 4.07-or-later, discuss-for-4.06.0 Oct 11, 2017
@lpw25
Copy link
Contributor

lpw25 commented Oct 11, 2017

The change seems obviously sound, since the code for expansive expressions should always be valid for non-expansive ones. The previous use of the check was definitely incorrect since non-expansive gives no guarantees about purity. The only question is whether this might somehow affect performance for cases where the expression is both non-expansive and is pure.

I think it's probably fine because an identifier being let bound won't make any difference and that will be by far the most common case. Also, code undergoing this transformation is probably quite rare anyway.

So I approve of merging this, but maybe give Jacques an opportunity to comment as I think the original code was his.

@damiendoligez damiendoligez modified the milestones: discuss-for-4.06.0, 4.06.0 Oct 11, 2017
@garrigue
Copy link
Contributor

One might want to check whether this adversely affects the size of code for lablgtk, which is a big user of this feature. (This said, in most cases the argument is a method call, which is not nonexpansive anyway).
Basically, I agree with @lpw25 's assessment.

@damiendoligez
Copy link
Member

@alainfrisch this PR will go to 4.06. Please write the Changes entry.

@alainfrisch
Copy link
Contributor Author

Changes entry added.

Copy link
Contributor

@garrigue garrigue left a comment

Choose a reason for hiding this comment

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

This change is certainly correct from a semantical point of view, and I agree with @lpw25 that in theory simplify should remove the overhead. Still would be a good idea to check that there are no performance regressions.

@alainfrisch
Copy link
Contributor Author

@garrigue You mention that lablgtk is a heavy user of the feature. Do you have some specific benchmark you could try?

@garrigue
Copy link
Contributor

The problem is not performance, just code size. So compiling the library before and after the fix, and check the size of the .cma and .a should be sufficient.

@mshinwell
Copy link
Contributor

I would like to request that we merge this fix without further delay. I don't really understand why benchmarking the code size of lablgtk is pertinent in the face of a bug involving miscompilation of side effects on an example as trivial as the one in MPR#7657. We need to check the performance / code size of the release as a whole in any case.

@damiendoligez damiendoligez merged commit 3d5e3a7 into trunk Oct 18, 2017
damiendoligez pushed a commit that referenced this pull request Oct 18, 2017
…nal arguments (#1424)

Fix MPR7657: ensure CBV semantics when eta-expanding function to eliminate optional arguments.
@damiendoligez
Copy link
Member

Merged and cherry-picked [4.06 a55915f].

We can check the lablgtk code size in parallel with other testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants