-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
New unreachable token value warning #2066
Conversation
@@ -96,8 +89,7 @@ public void checkActionRedefinitions(List<GrammarAST> actions) { | |||
nameNode = (GrammarAST) ampersandAST.getChild(0); | |||
if (ampersandAST.getChildCount() == 2) { | |||
name = nameNode.getText(); | |||
} | |||
else { | |||
} 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.
hi. please use same style. }
on line by themselves
* for repeated defs. | ||
*/ | ||
public void checkForLabelConflicts(Collection<Rule> rules) { | ||
/** |
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.
are you changing whitespace by mistake?
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.
Сonversely I replaced all whitespaces on tabs (respect to general project style).
@@ -341,42 +318,130 @@ public void checkForModeConflicts(Grammar g) { | |||
} | |||
} | |||
|
|||
public void checkForUnreachableTokens(Grammar g) { |
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.
can you explain in a comment your strategy for finding unreachable tokens? I also need to review that strategy to know if it is correct. :)
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.
Done :)
Please wait, do not merge. I want to implement yet another check. |
Probably the complexity can be reduced down to linear. I'll take a look until the end of the week. |
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.
Need to remove the unrelated formatting changes from this.
@@ -223,11 +207,11 @@ private void checkForTypeMismatch(Rule r, LabelElementPair prevLabelPair, LabelE | |||
labelPair.type + "!=" + prevLabelPair.type); | |||
} | |||
if (!prevLabelPair.element.getText().equals(labelPair.element.getText()) && | |||
(prevLabelPair.type.equals(LabelType.RULE_LABEL) || prevLabelPair.type.equals(LabelType.RULE_LIST_LABEL)) && | |||
(labelPair.type.equals(LabelType.RULE_LABEL) || labelPair.type.equals(LabelType.RULE_LIST_LABEL))) { | |||
(prevLabelPair.type.equals(LabelType.RULE_LABEL) || prevLabelPair.type.equals(LabelType.RULE_LIST_LABEL)) && |
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.
📝 Previous indentation was correct here:
antlr4/runtime/Java/nb-configuration.xml
Line 36 in 64ae36b
<org-netbeans-modules-editor-indent.text.x-java.CodeStyle.project.indent-shift-width>4</org-netbeans-modules-editor-indent.text.x-java.CodeStyle.project.indent-shift-width> |
} | ||
} | ||
|
||
protected void checkReservedNames(Collection<Rule> rules) { | ||
for (Rule rule : rules) { | ||
if (reservedNames.contains(rule.name)) { | ||
errMgr.grammarError(ErrorType.RESERVED_RULE_NAME, g.fileName, ((GrammarAST)rule.ast.getChild(0)).getToken(), rule.name); | ||
errMgr.grammarError(ErrorType.RESERVED_RULE_NAME, g.fileName, ((GrammarAST) rule.ast.getChild(0)).getToken(), rule.name); |
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.
📝 Previous spacing was correct here:
antlr4/runtime/Java/nb-configuration.xml
Line 43 in 64ae36b
<org-netbeans-modules-editor-indent.text.x-java.CodeStyle.project.spaceAfterTypeCast>false</org-netbeans-modules-editor-indent.text.x-java.CodeStyle.project.spaceAfterTypeCast> |
GrammarAST arg = (GrammarAST)ref.getFirstChildWithType(ANTLRParser.ARG_ACTION); | ||
if ( arg!=null && (r==null || r.args==null) ) { | ||
GrammarAST arg = (GrammarAST) ref.getFirstChildWithType(ANTLRParser.ARG_ACTION); | ||
if (arg != null && (r == null || r.args == null)) { |
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.
📝 Spacing here was generally unspecified (@parrt uses the previous spacing, while I use the updated spacing). Generally accepted practice became not changing it unless you were rewriting the line.
@@ -522,15 +522,15 @@ | |||
*/ | |||
USE_OF_BAD_WORD(134, "symbol <arg> conflicts with generated code in target language or runtime", ErrorSeverity.ERROR), | |||
/** | |||
* Compiler Error 134. | |||
* Compiler Error 183. |
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.
💭 Wasn't this part of another pull request?
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.
Yes, it is, but already merged.
"TOKEN7: 'qwer'+;\n" + | ||
"TOKEN8: 'a' 'b' | 'b' | 'a' 'b';\n" + | ||
"fragment\n" + | ||
"TOKEN9: 'asdf' | 'qwer' | 'qwer';\n" + |
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.
💡 Should include tokens with characters that need escapes, to make sure the values are properly escaped in the error messages.
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.
Done.
…rror type. tabs normalized.
9ebf9e6
to
763eccc
Compare
Ok, are we ready to go with this merge? |
@sharwell please check last fixes. |
@KvanTTT How many grammars have you run this on? |
All grammars from an official grammars-v4 repository, see my first comment: #2066 (comment) |
Partial fix for #1072 (see unit tests).
I tested my version on grammars-v4 repository and found a couple of such warnings:
Also I discovered and fixed such warnings in ANTLR unit tests itself (testInvalidLexerCommand, testLexerCommandArgumentValidation).