Skip to content

Java: Port C++ query cpp/continue-in-false-loop to Java.#2175

Merged
yh-semmle merged 2 commits intogithub:masterfrom
aschackmull:java/continue-in-false-loop
Oct 25, 2019
Merged

Java: Port C++ query cpp/continue-in-false-loop to Java.#2175
yh-semmle merged 2 commits intogithub:masterfrom
aschackmull:java/continue-in-false-loop

Conversation

@aschackmull
Copy link
Contributor

This query seems like it would be nice to have for Java as well. It doesn't have many results, but they tend to be good. See https://lgtm.com/query/5394831352853525197/ for two example results, https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java#L15212 is a particularly nice TP with a comment that makes it very clear that there is a mistake.

@aschackmull aschackmull requested review from a team and felicitymay as code owners October 23, 2019 09:16
@jbj
Copy link
Contributor

jbj commented Oct 23, 2019

The C++ version also comes with tests that you can port. I think your port will currently classify https://github.com/Semmle/ql/blob/30483db621a480b24a41bde0377a8db2d34de7da/cpp/ql/test/query-tests/Likely%20Bugs/ContinueInFalseLoop/test.cpp#L88 as bad, but we think it's good because using continue there really is different from using break.

@aschackmull
Copy link
Contributor Author

I haven't seen that case in Java code, but if I saw it, I'd prefer a labelled break over continue. And I think flagging it makes sense as there's still a significant chance that it was a mistake.

felicitymay
felicitymay previously approved these changes Oct 23, 2019
Copy link
Contributor

@felicitymay felicitymay left a comment

Choose a reason for hiding this comment

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

Thanks for the ping and the change note. This all LGTM.

@yh-semmle yh-semmle merged commit 80fd5b2 into github:master Oct 25, 2019
@aschackmull aschackmull deleted the java/continue-in-false-loop branch October 25, 2019 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants