-
-
Notifications
You must be signed in to change notification settings - Fork 741
Regex bugfix for staging (same as pull #1040) #1041
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
Conversation
A leftover from semantically incorrect optimization in std.regex
@@ -5559,7 +5554,7 @@ enum OneShot { Fwd, Bwd }; | |||
recycle(t); | |||
t = worklist.fetch(); | |||
if(t) | |||
break; | |||
break; | |||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant flow control
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not :)
The break goes out of switch case. while return bump us out of the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant the else could (and IMHO should) be removed:
if (t)
break;
return;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excuse me then. Indeed there is no need for else.
Updated & synced pull #1040 |
I think the release manager (i.e. Walter for now) should cherry-pick them on the staging branch. |
That was my understanding too if we are to have a simpler process. But I'd rather have it confirmed. Even then there has to be a note on each commit that is meant for pulling. Maybe we should mark them [for staging] or some other tag in front of commit description. Because putting burden of sorting out all of commits on to the release manger is just plain wrong. |
I don't think we should put [for staging] or similar tags in the commit messages, as the commit author can't actually decide whether something makes it into the staging branch or not. Some of the "you fools aren't using Git correctly" people on the forums are going to hate me for this, but I think the simplest solution is just for the author to ping the release manager after a given pull request commit has been merged to master. This is how it is e.g. done in the LLVM project, and it seems to work for them. |
@WalterBright leaving to you the decision to pull anything into staging. |
Okay, I'm truning this down. The list of commits can be looked up in pull #1040 if need be. |
Same as #1040. It's purely a bugfix.
BTW what is the current stance on staging - should we do pulls for it separately like this?
(or should it be cherry-picked manually later from master?)