Better message for unexpected trailing commas in parameter lists#2954
Better message for unexpected trailing commas in parameter lists#2954axic merged 1 commit intoargotorg:developfrom wadealexc:develop
Conversation
libsolidity/parsing/Parser.cpp
Outdated
| { | ||
| parameters.push_back(parseVariableDeclaration(options)); | ||
|
|
||
| if(m_scanner->currentToken() == Token::Comma && m_scanner->peekNextToken() == Token::RParen){ |
There was a problem hiding this comment.
It looks like this check only happens at the beginning of the parameter list, instead of the end.
Could you please add some tests to verify that?
Also, you accidentally added a binary file to this PR. Please remove it and rebase.
There was a problem hiding this comment.
Right, fixed that. Will add boost tests and re-request
| # true and continue as normal, either processing further commands in a script | ||
| # or returning the cursor focus back to the user in a Linux terminal. | ||
| ../cpp-ethereum/build/eth/eth --test -d /tmp/test & | ||
| $ETH_PATH --test -d /tmp/test & |
There was a problem hiding this comment.
This seems to fix a change introduced by an earlier commit of yours. Can you please squash the two commits?
There was a problem hiding this comment.
Trying to squash all of my commits, accidentally created more. Could you point me in the right direction for cleaning this up? I haven't submitted a PR to a repository before so I'm new to squashing/merging/etc.
|
|
||
| while (m_scanner->currentToken() != Token::RParen) | ||
| { | ||
| expectToken(Token::Comma); |
There was a problem hiding this comment.
How about replacing those two checks with a single one before this expectToken? I think that should work.
There was a problem hiding this comment.
Also please follow the coding style (space after if, before {, also no brackets needed if the body of a conditional is a single expression).
|
The tests look great and comprehensive! |
axic
changed the title
test/libsolidity/SolidityParser.cpp
Outdated
| function(uint a,) {} | ||
| } | ||
| )"; | ||
| CHECK_PARSE_ERROR(text, "Unexpected trailing comma in arguments"); |
There was a problem hiding this comment.
I know this was the message suggested in the issue, but perhaps Unexpected trailing comma in parameter list is more universal given it is used on multiple places.
@chriseth @federicobond what do you think?
There was a problem hiding this comment.
Sure, let's go ahead with parameter list.
|
Changed boost tests and error message to "Unexpected trailing comma in parameter list" I've tried to squash my commits, but it keeps layering more on. I think as members of the repo you can squash when you merge. Otherwise, is there a different way for me to squash these? |
|
@wadealexc squashed and rebased in your branch, please do not update it, it should be ready to merge now. |
4 participants
Fixes #2579.
Improve error message for trailing commas in function signatures -
Trailing commas in function arguments, return parameters, event arguments, and modifier arguments now display the error message "Unexpected trailing comma in arguments"