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

Re-enable check against FallThrough violations #364

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

Re-enable check against FallThrough violations #364

wants to merge 11 commits into from

Conversation

regisd
Copy link
Member

@regisd regisd commented Sep 22, 2018 •
edited

Fix #222
Fix #239

@regisd regisd requested a review from lsf37 as a code owner October 15, 2018 09:52
@regisd regisd mentioned this pull request Oct 18, 2018
Merged
@regisd regisd added this to the 1.8.0 milestone Oct 18, 2018
@regisd regisd added the code quality Code health and clean-up label Oct 18, 2018
@regisd regisd self-assigned this Nov 23, 2019
@regisd regisd removed this from the 1.8.0 milestone Nov 27, 2019
lsf37
lsf37 approved these changes Dec 7, 2019
View reviewed changes
Copy link
Member

@lsf37 lsf37 left a comment

Choose a reason for hiding this comment

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

Looks good from my side.

@regisd
Copy link
Member Author

regisd commented Dec 8, 2019

IIRC, this can only be done with code generated with 1.8.0, hence can only be done in 1.9.0

@regisd regisd added this to the 1.9.0 milestone Dec 8, 2019
@lsf37
Copy link
Member

lsf37 commented Dec 9, 2019

IIRC, this can only be done with code generated with 1.8.0, hence can only be done in 1.9.0

Makes sense, let's leave this here until 1.8.0 is out.

@lsf37
Copy link
Member

lsf37 commented Mar 15, 2020

This seems to be passing even though bazel is still building with 1.7.0. Is this Ok to merge?

@regisd
Copy link
Member Author

regisd commented Mar 16, 2020

I'm afraid not. I tried to update to jflex-1.8 in the corp repository, and I get a number of errors. Sorry that I didn't take the time to investigate.

@lsf37
Copy link
Member

lsf37 commented Mar 17, 2020

No worries. Are the errors about fall-through or other jflex 1.8.0 things?

@regisd
Copy link
Member Author

regisd commented Apr 10, 2020

So, the scope of the suppress warning (either via @SuppressWarnings or via javacop -X:ep is only about FallThrough. I'm running a presubmit now.

@regisd
Copy link
Member Author

regisd commented Apr 14, 2020 •
edited

There are remaining violations on 1.8.0.

jflex/java/jflex/core/LexScan.java:2483: error: [FallThrough] Execution may fall through from the previous case; add a `// fall through` comment before this line if it was deliberate
            case 800: break;
            ^
    (see https://errorprone.info/bugpattern/FallThrough)

@regisd regisd marked this pull request as draft December 20, 2020 13:31
@regisd regisd removed this from the 1.9.0 milestone Dec 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsf37 lsf37 lsf37 approved these changes

Assignees

@regisd regisd

Labels
code quality Code health and clean-up
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable error-prone FallThrough test jflex has error-prone fall-through
2 participants
@regisd @lsf37