-
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
caseInsensitive lexer rule option #3437
Conversation
b614644
to
d763a26
Compare
d763a26
to
214a45b
Compare
2ee4d10
to
e8771c1
Compare
|
||
## Lexer Rule Options | ||
|
||
### caseInsensitive |
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'm not sure about the best location for caseInsensitive
lexer rule option. Maybe it makes sense to move to https://github.com/antlr/antlr4/blob/master/doc/options.md#rule-options
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.
maybe a reference in one spot to the other?
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, I made a reference in options.md: https://github.com/antlr/antlr4/pull/3437/files#diff-6420f854d462e04fe86b98f8bd495e3ec6f42f94121c39a37d228ba74ea91052R114
public static final Set<String> parserOptions = new HashSet<String>(); | ||
public static final String caseInsensitiveOptionName = "caseInsensitive"; | ||
|
||
public static final Set<String> ParserOptions = new HashSet<String>(); |
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 think for consistency we should keep this as initial lowercase, just like the others.
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've encountered a mess with lower/UPPER case identifiers: https://github.com/antlr/antlr4/blob/master/tool/src/org/antlr/v4/tool/Grammar.java#L89-L95 So, you think it's better to use lower case for all options, do you?
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.
well I reserve the first capital letter for types like classes... at the very least we should be consistent at this point
sorry.. I messed up the build of this PR when I tried to resolve the conflict. I missed:
in the lexer atn factory. looking at it locally but maybe you could fix it in your branch. |
No problem, let me resolve conflicts and repush it again. |
cool. it's looking like a very simple change! kudos. |
okay, everything looks good to me except for that minor capitalization thing. |
71dbd36
to
a853356
Compare
* add options to lexer rules per antlr/antlr4#3437 * Fixes #525, clean up calls to antlr tool. upgrade lib versions. Seems to work with 2021.3.3 * Fixes #518 * update doc Signed-off-by: Terence Parr <parrt@antlr.org> * Allow earlier versions of intellij * add noteAbout how to remove a deprecated function in the future; I couldn't figure out how to make it work at the moment. * update doc Signed-off-by: Terence Parr <parrt@antlr.org> * remove dead code * let gradle see antlr snapshots * test with latest intellij
It was quite simple to implement, but it should be merged after #3435