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

Update Protobuf3.g4 #3949

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update Protobuf3.g4 #3949

wants to merge 1 commit into from

Conversation

maxwell2011
Copy link

Line 465, corrected PROTO3_LIT_DOBULE to PROTO3_LIT_DOUBLE

Line 465, corrected PROTO3_LIT_DOBULE to PROTO3_LIT_DOUBLE
@kaby76
Copy link
Contributor

kaby76 commented Jan 28, 2024

This change is an incomplete rename. You need to change not only the defining occurrence of PROTO3_LIT_DOBULE but all applied occurrences too. https://github.com/antlr/grammars-v4/actions/runs/7683358509/job/20942992775?pr=3949#step:24:61

(The build should be changed to treat warnings as errors, but it would break a good number of grammars.)

Also, there seem to be a dozen or so useless parentheses in the grammar that never got cleaned up with the addition of the string literal lexer rules/folding (#2090).

$ bash /c/Users/Kenne/Documents/GitHub/g4-scripts/find-useless-parentheses.sh
CSharp 0 Protobuf3.g4 success 0.0494654
running parser and computing useless parentheses.
CSharp 0 Protobuf3.g4 success 0.0488163
Found useless parentheses in grammars...
Protobuf3.g4:L172:     : ident EQ (MINUS)? intLit enumValueOptions? SEMI
                                  ^
Protobuf3.g4:L229:     : RPC rpcName LP (STREAM)? messageType RP RETURNS LP (STREAM)? messageType RP (
                                        ^
Protobuf3.g4:L229:     : RPC rpcName LP (STREAM)? messageType RP RETURNS LP (STREAM)? messageType RP (
                                                                            ^
Protobuf3.g4:L295:     : (DOT)? (ident DOT)* messageName
                         ^
Protobuf3.g4:L299:     : (DOT)? (ident DOT)* enumName
                         ^
Protobuf3.g4:L532:     : ('\'' ( CHAR_VALUE)*? '\'')
                         ^
Protobuf3.g4:L532:     : ('\'' ( CHAR_VALUE)*? '\'')
                               ^
Protobuf3.g4:L533:     | ( '"' ( CHAR_VALUE)*? '"')
                         ^
Protobuf3.g4:L533:     | ( '"' ( CHAR_VALUE)*? '"')
                               ^
Protobuf3.g4:L561:     : (DECIMALS DOT DECIMALS? EXPONENT? | DECIMALS EXPONENT | DOT DECIMALS EXPONENT?)
                         ^
Protobuf3.g4:L581:     : ([1-9]) DECIMAL_DIGIT*
                         ^
01/28-06:17:32 ~/issues/g4-3949/protobuf3

@teverett
Copy link
Member

teverett commented Feb 9, 2024

@kaby76 are changes needed, or is this ok to merge?

@kaby76
Copy link
Contributor

kaby76 commented Feb 10, 2024

@teverett This PR is NOT COMPLETE. The renaming of PROTO3_LIT_DOBULE to PROTO3_LIT_DOUBLE was done here:
https://github.com/maxwell2011/grammars-v4/blob/0036eaf4ac650e45c445c301cf326fbd316b9a08/protobuf3/Protobuf3.g4#L465

But, it was not renamed in these two places:
https://github.com/maxwell2011/grammars-v4/blob/0036eaf4ac650e45c445c301cf326fbd316b9a08/protobuf3/Protobuf3.g4#L27

https://github.com/maxwell2011/grammars-v4/blob/0036eaf4ac650e45c445c301cf326fbd316b9a08/protobuf3/Protobuf3.g4#L309

The Antlr tool outputs a warning "warning(125): Protobuf3.g4:27:37: implicit definition of token PROTO3_LIT_DOBULE in parser", but we don't quit on warnings. The grammar was changed, but it's not clear if there is input that tests this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants