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 17646 dmd segfaults on missing [tuple] foreach body in import #6992

Closed
wants to merge 1 commit into from

Conversation

UplinkCoder
Copy link
Member

As per title the reason this happens is because an UnrolledLoopStatement (which is what Tuple-foreach becomes) is not technically required to have statements in it's body.
However it seems so far this was not trigged.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jul 13, 2017

Thanks for your pull request, @UplinkCoder!

Bugzilla references

Auto-close Bugzilla Description
17646 dmd segfaults on missing foreach body in import

@@ -739,7 +739,8 @@ private extern (C++) final class StatementSemanticVisitor : Visitor
}
st.push(new ExpStatement(loc, var));

st.push(fs._body.syntaxCopy());
if (fs._body)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ {5}/ {4}/ :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I should use an editor with auto-indent :)

@UplinkCoder
Copy link
Member Author

@andralex could you slap an auto-merge on this one ?
Thanks.

@wilzbach
Copy link
Member

could you slap an auto-merge on this one ?

Any reason why this isn't based on stable?

@UplinkCoder UplinkCoder changed the base branch from master to stable July 13, 2017 16:38
@UplinkCoder UplinkCoder changed the base branch from stable to master July 13, 2017 16:39
@UplinkCoder
Copy link
Member Author

waahh. what did dlang bot do.
There you have the answer.

@UplinkCoder
Copy link
Member Author

closing in favor of version against stable

@mathias-lang-sociomantic
Copy link
Contributor

You know you can just change the target of this P.R.

@wilzbach
Copy link
Member

waahh. what did dlang bot do.
There you have the answer.

Not sure what exactly happened, but in general it sends a friendly reminder when a regression is targeted at master and not stable (like it did here in the beginning), because sadly this still happens way too often.

I am only on my phone atm, but it could be that you changed the priority of the issue and the bot updated his message consequently?

@UplinkCoder
Copy link
Member Author

@mathias-lang-sociomantic I did just that, changed the target.
It resulted in a PR with 201 commits.

@John-Colvin
Copy link
Contributor

changing the target isn't actually that useful in these cases seeing as it doesn't change the source branch at all. It's only really useful when you accidentally did the merge request to the wrong branch, not if you started your commits from the wrong branch.

@wilzbach
Copy link
Member

It resulted in a PR with 201 commits.

Yes you need to rebase yourself. E.g.

git rebase - --onto=upstream/stable

We really need to teach the core contributors better git skills. Maybe at DConf18 there should be a talk/workshop about git?😉

@mathias-lang-sociomantic
Copy link
Contributor

mathias-lang-sociomantic commented Jul 13, 2017

@mathias-lang-sociomantic I did just that, changed the target.
It resulted in a PR with 201 commits.

Yes, then you have to rebase your P.R. onto stable

Edit: Too slow. I agree 100% with @wilzbach , there is really a problem if core contributors do not understand basic git commands.

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.

5 participants