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.

Sign up for GitHub

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

Jump to bottom

Some bugfixes to the tool #3310

Merged
merged 3 commits into from
Oct 24, 2021
Merged

Some bugfixes to the tool #3310

merged 3 commits into from
Oct 24, 2021

Conversation

KvanTTT
Copy link
Member

@KvanTTT KvanTTT commented Oct 17, 2021

No description provided.

@KvanTTT KvanTTT mentioned this pull request Oct 17, 2021
Closed
parrt
parrt reviewed Oct 17, 2021
View reviewed changes
tool/src/org/antlr/v4/automata/LexerATNFactory.java Outdated
if (ruleCommands.contains("more")) {
firstCommand = "more";
}
else if (ruleCommands.contains("type")) {
} else if (ruleCommands.contains("type")) {
Copy link
Member

Choose a reason for hiding this comment

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

looks like this got reformatted making diff hard to read

Copy link
Member

Choose a reason for hiding this comment

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

also is this java 7?

Copy link
Member Author

Choose a reason for hiding this comment

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

looks like this got reformatted making diff hard to read

I've just applied the transform suggested by IDEA.

also is this java 7?

Yes, it's java 7:

In Java SE 7 and later, you can use a String object in the switch statement's expression.

https://docs.oracle.com/javase/tutorial/java/nutsandbolts/switch.html

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I just never change software unless it's broken or needs to be enhanced. Is this such a case?

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 removed useless and incorrect comparison and replaced if .. else blocks with switch. I can revert the last transform if it's more convenient for you.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's reduce changes just in case it changes logic. thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @KvanTTT when you have time, could you reverse that if->switch transform? thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I've already reversed it. Now I'm working on another fix that will be done closer to the weekend.

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 decided to move other fixes to another pull request. So, this request is ready for review.

@parrt parrt added this to the 4.9.3 milestone Oct 24, 2021
@parrt parrt merged commit e8599bf into antlr:master Oct 24, 2021
@KvanTTT KvanTTT deleted the tool-bugfixes branch October 25, 2021 13:52
@parrt
Copy link
Member

parrt commented Oct 26, 2021

Hi @KvanTTT getting an error now. Is this related to this PR?

https://ci.appveyor.com/project/parrt/antlr4/builds/41296106/job/5707xytwjo4y7l42

[ERROR] error(174): org\antlr\v4\test\runtime\java\api\perf\graphemes.g4:14:4: string literals and sets cannot be empty: []
[ERROR] C:\projects\antlr4\org\antlr\v4\test\runtime\java\api\perf\graphemes.g4 [14:4]: string literals and sets cannot be empty: []
[ERROR] error(174): org\antlr\v4\test\runtime\java\api\perf\graphemes.g4:14:77: string literals and sets cannot be empty: []
[ERROR] C:\projects\antlr4\org\antlr\v4\test\runtime\java\api\perf\graphemes.g4 [14:77]: string literals and sets cannot be empty: []
[ERROR] Failed to execute goal org.antlr:antlr4-maven-plugin:4.9.3-SNAPSHOT:antlr4 (default) on project antlr4-runtime-testsuite: ANTLR 4 caught 2 build errors. -> [Help 1]

@KvanTTT
Copy link
Member Author

KvanTTT commented Oct 27, 2021 •
edited
Loading

Probably, but it's strange that there were no errors while merging the PR. I'll take a look.

@KvanTTT
Copy link
Member Author

KvanTTT commented Oct 27, 2021 •
edited
Loading

It's weird, but I can not reproduce the error on the latest master. ANTLR handles this file without errors. Also, CI was working after merging this PR: https://ci.appveyor.com/project/parrt/antlr4/builds/41266387 Maybe some other stuff broke the tool.

@parrt
Copy link
Member

parrt commented Oct 27, 2021

@KvanTTT let me try running it again locally.

@KvanTTT KvanTTT mentioned this pull request Feb 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parrt parrt parrt left review comments

Assignees
No one assigned
Labels
comp:tool error-handling
Projects
None yet
Milestone
4.9.3
Development

Successfully merging this pull request may close these issues.
2 participants
@KvanTTT @parrt