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

Fix Issue 16317 - Wrong binop evaluation/load order when optimizing #8076

Closed
wants to merge 1 commit into from

Conversation

LemonBoy
Copy link
Contributor

Nothing to see here (for now), move along.
Let's see if 16317 is gone for good.

@dlang-bot
Copy link
Contributor

dlang-bot commented Mar 25, 2018

Thanks for your pull request and interest in making D better, @LemonBoy! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
16317 critical Wrong binop evaluation/load order when optimizing

@WalterBright WalterBright changed the title binop folding fix Issue 16317 - Wrong binop evaluation/load order when optimizing Mar 25, 2018
@WalterBright
Copy link
Member

Please use the format: "fix Issue NNNN ..." for PR subjects that fix Issue NNNN. This triggers the dlangbot to do the right thing and connect it. It's also necessary to insert the link to this PR as a comment on the bugzilla issue. (I have already done it for this PR.) All this is necessary for cross referencing purposes.

@WalterBright
Copy link
Member

@wilzbach
Copy link
Member

(FYI @LemonBoy: @dlang-bot looks at the commit message - not the title)

@LemonBoy
Copy link
Contributor Author

@WalterBright as you may have noticed there's no description of the problem nor of the solution implemented, there's no proper commit message but there's a suggestive "Nothing to see here (for now), move along." in the first message.
I only needed to run this commit trough the testing pipeline to see whether my assumptions about the code are correct or not, I'll follow up with a proper patch and commit message soon.

We have to be extra careful not to fold an assignment+op combo into a
single operation if the right hand side may modify one of the variables
@wilzbach wilzbach changed the title fix Issue 16317 - Wrong binop evaluation/load order when optimizing Fix Issue 16317 - Wrong binop evaluation/load order when optimizing Mar 25, 2018
@WalterBright
Copy link
Member

I only needed to run this commit trough the testing pipeline to see whether my assumptions about the code are correct or not, I'll follow up with a proper patch and commit message soon.

Thanks for the explanation (and the work!). In general, if a PR is not ready for review, put WIP in the title and we won't bother you until it's ready :-)

@RazvanN7
Copy link
Contributor

@LemonBoy what's the status on this?

@RazvanN7
Copy link
Contributor

@LemonBoy ping

@RazvanN7
Copy link
Contributor

I will close this for now as it appears that the purpose of it was to test some assumptions. @LemonBoy feel free to reopen if you will start working again on it.

@RazvanN7 RazvanN7 closed this May 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants