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

Issue 15450 - ICE during CTFE of legit function #5312

Closed
wants to merge 3 commits into from

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Dec 16, 2015

ForeachStatement didn't set LabelStatement.gotoTarget correctly, so it had broke the jump behavior during CTFE.

Don't use sc2 for the checkLabeledLoop argument in ForeachStatement.semantic, because sc2.slabel is not same with sc.slabel. (it's reset to null in sc.push()).

Use `scope(exit)`, then we can reduce goto jumps.
`ForeachStatement` didn't set `LabelStatement.gotoTarget` correctly, so it had broke the jump behavior during CTFE.

Don't use `sc2` for the `checkLabeledLoop` argument in `ForeachStatement.semantic`, because `sc2.slabel` is not same with `sc.slabel`. (it's reset to `null` in `sc.push()`).
@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
15450 ICE during CTFE of legit function

@WalterBright
Copy link
Member

Please put refactorings in separate PRs. The actual bug fix here is just a handful of lines, but it comes with 240 more lines of changes. Even b09f2d8 is mostly refactorings.

@9rnsr
Copy link
Contributor Author

9rnsr commented Dec 24, 2015

@WalterBright Refactoring PR is normally left, so subsequent bugfix would also be left. So in order to fix the bug quickly, I should combine the refactorings and bugfix in a PR.

In this case, I cannot understand why the refactoring part should go into other PR. It has no benefit.

@9il
Copy link
Member

9il commented Dec 30, 2015

I am not a DMD contributor, but I'll try to be objective.

@9rnsr fixes a lot issues, and he is doing this quick.
I understand that existing DMD contributors already understand DMD front-end. However for a common Phobos contributor this code looks very inconvenient end this force him to do not make PRs and only reports bugs. So this work is important.

@WalterBright I understand that it tooks a lot of yours time to review refactoring and changes. But D/Phobos idiomatic style allows other D contributors to made PRs by they self, and probably some of them will be a part of DMD reviewers team in the future. So you will be able to spend more time for more important questions rather than review :-)

In addition, I am worrying about #5309

Thank you all for fastest compiler ever!

@WalterBright
Copy link
Member

I have explained the benefit multiple times in these PRs.

@WalterBright
Copy link
Member

The other issue I have with this is the use of scope(exit). The trouble is that it sets up an exception handler frame to do this, and that's slower, even though no exceptions are thrown (none of the functions in the body are marked 'nothrow'). I want to be very parsimonious in adding in things that may slow the compiler down.

We should also improve the compiler so using scope is less impactful, but we aren't there yet.

So, I'll pull this if b09f2d8 is the whole PR, and the scope(exit) refactoring is not part of it.

@WalterBright
Copy link
Member

Rebased and rebooted as #5336 as Kenji's bug fix but without the refactoring and reformatting.

WalterBright added a commit to WalterBright/dmd that referenced this pull request Jan 8, 2016
andralex added a commit that referenced this pull request Jan 8, 2016
Reboot #5312: fix Issue 15450 - ICE during CTFE of legit function
adamdruppe pushed a commit to adamdruppe/dmd that referenced this pull request Apr 18, 2016
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.

4 participants