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

jflex has error-prone fall-through #222

Open
regisd opened this issue Nov 1, 2017 · 9 comments · May be fixed by #364
Open

jflex has error-prone fall-through #222

regisd opened this issue Nov 1, 2017 · 9 comments · May be fixed by #364
Assignees
Labels
code quality Code health and clean-up

Comments

@regisd
Copy link
Member

regisd commented Nov 1, 2017

jflex/jflex-unicode-maven-plugin/target/generated-sources/jflex/jflex/ArchaicLineBreakScanner.java:702: error: [FallThrough] Execution may fall through from the previous case; add a // fall through comment before this line if it was deliberate

          case 11: break;
          ^

see http://errorprone.info/bugpattern/FallThrough

@regisd regisd added the code quality Code health and clean-up label Nov 1, 2017
regisd added a commit that referenced this issue Nov 1, 2017
Note that FallThrough violations have been reduced to warnings.

issue #222
@lsf37
Copy link
Member

lsf37 commented Nov 2, 2017

The fall-through violations seem to be in generated code -- we could automatically add the // fall through comment in the generator to indicate that this is on purpose.

regisd added a commit that referenced this issue Nov 3, 2017
* Add google error-prone in compile step.
  Note that FallThrough violations have been reduced to warnings. See issue #222
* Enable error-prone in the java 8 profile only, because the plugin was compiled for java 8, and cannot be used in openjdk7.
@regisd regisd added this to the release 1.8.0 milestone Nov 3, 2017
@regisd regisd self-assigned this Nov 3, 2017
@regisd regisd modified the milestones: 1.8.0, 1.7.0 Nov 4, 2017
@regisd regisd closed this as completed Nov 4, 2017
@regisd
Copy link
Member Author

regisd commented Nov 9, 2017

Unfortunately, that's not enough.

At the end of emitActions, I have added a fall-through comment

  println("            { " + action.content);
  println("            } ");
  println("            // fall through ");

The problem is that action.content can contain a return or a throw statement ; in which case the fall-through comment is not legitimate anymore.

@regisd regisd reopened this Nov 9, 2017
@lsf37
Copy link
Member

lsf37 commented Nov 9, 2017

Right, that's actually what that specific fall through is for: to guard for the cases where there is no return or throw and the scan just continues. Do we get warnings for fall through comments that are spurious? If not, we could add another line with something like
// allowed to fall through if action does not return or throw

@wrprice
Copy link

wrprice commented Aug 28, 2018

Curious... why use fall through to a new case instead of adding an explicit break; ?

@regisd
Copy link
Member Author

regisd commented Aug 28, 2018

// fall through is the exact opposite of break. It means the case falls to the following case statement, and that the developer has not forgotten the break.

@wrprice
Copy link

wrprice commented Aug 28, 2018

I understand, but I noticed quite a few cases that fall-through to cases that only break; and do nothing else. I was just about to edit my post because the answer is probably because you don't know whether the action code included a return or break, and without examining it the safest thing is to fall-through to a separate case that breaks. Blindly inserting a break after an action could result in an unreachable statement otherwise.

Sorry for the noise.

regisd added a commit that referenced this issue Aug 28, 2018
s/though/trough

Issue #222
regisd added a commit that referenced this issue Aug 29, 2018
s/though/through

Issue #222
@regisd regisd closed this as completed Sep 3, 2018
@regisd
Copy link
Member Author

regisd commented Sep 22, 2018

There are still violations
https://travis-ci.org/jflex-de/jflex/jobs/431900782

@regisd regisd reopened this Sep 22, 2018
@regisd regisd modified the milestones: 1.7.0, 1.7.1 Sep 22, 2018
@regisd
Copy link
Member Author

regisd commented Oct 15, 2018

Work around: Use javac option -Xep:FallThrough:WARN

@regisd
Copy link
Member Author

regisd commented Oct 18, 2018

I also believe that JFlex 1.7.0 has fall-through violations that 1.6.1 didn't have.

@regisd regisd modified the milestones: 1.7.1, 1.8.0 Nov 5, 2019
@regisd regisd removed this from the 1.8.0 milestone Nov 27, 2019
@regisd regisd linked a pull request Apr 10, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Code health and clean-up
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants