Skip to content

Issue 15163 - Parser bug on double function call#5165

Merged
ibuclaw merged 1 commit intodlang:masterfrom
9rnsr:fix15163
Oct 11, 2015
Merged

Issue 15163 - Parser bug on double function call#5165
ibuclaw merged 1 commit intodlang:masterfrom
9rnsr:fix15163

Conversation

@9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Oct 6, 2015

https://issues.dlang.org/show_bug.cgi?id=15163

Add allowAltSyntax parameter to isDeclarator, and disable it from parseStatement.

Add `allowAltSyntax` parameter to `isDeclarator`, and disable it from `parseStatement`.
@ibuclaw
Copy link
Member

ibuclaw commented Oct 11, 2015

My only request here would be to use self-documenting enums rather than continuing to use an integer for the needId parameter.

@9rnsr
Copy link
Contributor Author

9rnsr commented Oct 11, 2015

Walter doesn't like to contain both refactoring and bugfix in a PR. So for the quick bugfix, I usually keep such the magic interger numbers in old code. And after the bugfix, the refactoring change will be ignored usually by Walter, because he doesn't like continuous refactoring changes!

@ibuclaw
Copy link
Member

ibuclaw commented Oct 11, 2015

And after the bugfix, the refactoring change will be ignored usually by Walter, because he doesn't like continuous refactoring changes!

I could take that argument as ad-infintum. One day it's the number 3, the next week it's a PR documenting what it means to have the number 42 passed to the function. :-)

OK, I'll give this the 👍 for now, but I'll put strong emphasis on refactoring this in the next months. I'd have to reject anything which involves more magic numbers than I have fingers on my left hand (that excludes the thumb) though.

Regardless of Walter's preferences on PR etiquette, so long as you keep each relevant change as a separate commit, others who review on a commit-by-commit basis will understand what's going on.

@ibuclaw
Copy link
Member

ibuclaw commented Oct 11, 2015

Auto-merge toggled on

@yebblies
Copy link
Contributor

Walter doesn't like to contain both refactoring and bugfix in a PR. So for the quick bugfix, I usually keep such the magic interger numbers in old code. And after the bugfix, the refactoring change will be ignored usually by Walter, because he doesn't like continuous refactoring changes!

Yeah, I think it's better to have the refactoring separate to the bugfix PR too. I don't have your knowledge of some parts of the compiler, and the more lines changed the harder it is to review. I will happily merge a followup PR that updates it to use an enum if you want to do that.

ibuclaw added a commit that referenced this pull request Oct 11, 2015
Issue 15163 - Parser bug on double function call
@ibuclaw ibuclaw merged commit d2c607f into dlang:master Oct 11, 2015
@9rnsr
Copy link
Contributor Author

9rnsr commented Oct 11, 2015

@ibuclaw Thanks. I opened #5184 while this bugfix is in my brain.

@9rnsr
Copy link
Contributor Author

9rnsr commented Oct 11, 2015

Yeah, I think it's better to have the refactoring separate to the bugfix PR too.

@yebblies If a bugfix requires preparation refactorings for clean & understandable change, the refactoring PR will block following bugfix PR. I already have a bugfix PR #5009 as the example.

@9rnsr 9rnsr deleted the fix15163 branch October 14, 2015 00:48
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.

3 participants