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

caseInsensitive lexer rule option #3437

Merged
merged 1 commit into from
Dec 28, 2021

Conversation

KvanTTT
Copy link
Member

@KvanTTT KvanTTT commented Dec 26, 2021

It was quite simple to implement, but it should be merged after #3435

@KvanTTT KvanTTT force-pushed the caseInsensitiveLexerCommand branch from b614644 to d763a26 Compare December 26, 2021 22:46
doc/lexer-rules.md Outdated Show resolved Hide resolved
@KvanTTT KvanTTT force-pushed the caseInsensitiveLexerCommand branch from d763a26 to 214a45b Compare December 26, 2021 23:54
@KvanTTT KvanTTT marked this pull request as draft December 27, 2021 21:24
@parrt parrt added the lexers label Dec 27, 2021
@KvanTTT KvanTTT force-pushed the caseInsensitiveLexerCommand branch 2 times, most recently from 2ee4d10 to e8771c1 Compare December 28, 2021 15:17
@KvanTTT KvanTTT changed the title caseInsensitive lexer command caseInsensitive lexer rule option Dec 28, 2021
@KvanTTT KvanTTT marked this pull request as ready for review December 28, 2021 15:20

## Lexer Rule Options

### caseInsensitive
Copy link
Member Author

@KvanTTT KvanTTT Dec 28, 2021

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

Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public static final Set<String> parserOptions = new HashSet<String>();
public static final String caseInsensitiveOptionName = "caseInsensitive";

public static final Set<String> ParserOptions = new HashSet<String>();
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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

@parrt
Copy link
Member

parrt commented Dec 28, 2021

sorry.. I messed up the build of this PR when I tried to resolve the conflict. I missed:

	private final boolean caseInsensitive;

in the lexer atn factory. looking at it locally but maybe you could fix it in your branch.

@KvanTTT
Copy link
Member Author

KvanTTT commented Dec 28, 2021

sorry.. I messed up the build of this PR when I tried to resolve the conflict. I missed:

No problem, let me resolve conflicts and repush it again.

@parrt
Copy link
Member

parrt commented Dec 28, 2021

cool. it's looking like a very simple change! kudos.

@parrt
Copy link
Member

parrt commented Dec 28, 2021

okay, everything looks good to me except for that minor capitalization thing.

@KvanTTT KvanTTT force-pushed the caseInsensitiveLexerCommand branch from 71dbd36 to a853356 Compare December 28, 2021 20:58
@parrt parrt added this to the 4.9.4 milestone Dec 28, 2021
@parrt parrt added the options label Dec 28, 2021
@parrt parrt merged commit d8e1525 into antlr:master Dec 28, 2021
@KvanTTT KvanTTT deleted the caseInsensitiveLexerCommand branch December 28, 2021 22:36
@KvanTTT KvanTTT mentioned this pull request Dec 29, 2021
parrt added a commit to parrt/intellij-plugin-v4 that referenced this pull request Apr 8, 2022
parrt added a commit to antlr/intellij-plugin-v4 that referenced this pull request Apr 9, 2022
* 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 <[email protected]>

* 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 <[email protected]>

* remove dead code

* let gradle see antlr snapshots

* test with latest intellij
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants