Page MenuHomePhabricator

[clang-tidy] Add new checker for complex conditions with no meaning
Needs ReviewPublic

Authored by vabridgers on Wed, Jul 29, 5:24 PM.

Details

Summary

This checker finds cases where relational expressions have no meaning.
For example, (x <= y <= z) has no meaning, but just happens to parse to
something deterministic. (x <= y <= z) is parsed to ((x <= y) <= z).
The (x<=y) returns a boolean value which is promoted to an integral
context, 0 (for false) or 1 (for true). So when (x <= y) the expression
becomes (1 <= z) and when (x > y) the expression becomes (0 <= z). When
asked to give all warnings, gcc warns about this. While this may be the
intention in some programming contexts, it may not have been what the
programmer really wanted in all contexts. So it's best to implement this as a
tidy check to start with, get some experience using it, then perhaps promote
this check to a diagnostic.

Diff Detail

Event Timeline

vabridgers created this revision.Wed, Jul 29, 5:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Jul 29, 5:24 PM
vabridgers requested review of this revision.Wed, Jul 29, 5:24 PM

Correct some simple mistakes

Another update

I believe this is ready for review. Thanks!

Eugene.Zelenko retitled this revision from clang-tidy] Add new checker for complex conditions with no meaning to [clang-tidy] Add new checker for complex conditions with no meaning.Wed, Jul 29, 7:55 PM
Eugene.Zelenko added reviewers: alexfh, hokein, njames93.
Eugene.Zelenko added a project: Restricted Project.

May be this check belongs to bugprone module?

clang-tools-extra/docs/ReleaseNotes.rst
79

Please enclose comparison operators in double back-ticks.

clang-tools-extra/docs/clang-tidy/checks/misc-complex-conditions.rst
7

Please synchronize with statement in Release Notes.

9

Please enclose statements examples and 0/1 in double back-ticks.

45

fix-it, check.

I really think this should be in clang proper.
I have seen such patterns written in the wild,
they should be caught by just the compiler.

For the record X < Y < Z does have a mathematical meaning, Y is constrained between X and Z.
However in the context of C the expression isnt parsed like that.
If someone writes this they likely wanted (X < Y) && (Y < Z)
For this specific check as you pointed out we wouldn't want to make that assumption though there is a case for adding notes to silence the warning by wrapping one of the comparisons in parenthesis.

It appears that this check fire multiple times for the case of W < X < Y < Z
Once for (W < X < Y) < Z and another time for (W < X) < Y
This again likely wont be visible as the warning currently is emitted at the start of the expression

clang-tools-extra/clang-tidy/misc/ComplexConditionsCheck.cpp
29–34

The If looks suspicious, as you match on a BinaryOperator, the If will always be nullptr and should likely be removed.
Likewise Statement will match but should likely be changed to BinOp as that's what it is.

35

Elide braces

Thanks all for the prompt and actionable comments. I will address all comments directly and 1-1, but would like to bottom out on one point before driving this forward. @lebedev.ri commented this should be in the Clang front end rather than a tidy check. Can we reach a consensus on that comment before I proceed with either a tidy check renamed to bugprone (comment from @Eugene.Zelenko ) or implement this check in the front end, enabled only when all warnings are enabled? I had considered both approaches when I started this, and thought the tidy checker was the best first step.

Also, @njames93, I take your point the expression does have a mathematical meaning. I was following the gcc warning message for the same expression, but we can cast the warning however we like :) Do you have a suggestion about the warning? I like the general idea of a suggestion to use parenthesis to remove the ambiguity.

Thanks! - Vince

vabridgers marked 6 inline comments as done.

Address most review comments.

I believe all review comments have been address, except for the discussion on implementing this in the CFE or as a tidy check.

Eugene.Zelenko added inline comments.Thu, Jul 30, 8:27 AM
clang-tools-extra/docs/ReleaseNotes.rst
79

Double back-ticks, not single ones.

clang-tools-extra/docs/clang-tidy/checks/bugprone-complex-conditions.rst
6 ↗(On Diff #281937)

Double back-ticks, not single ones.

Address backticks in rst files

vabridgers marked 2 inline comments as done.Thu, Jul 30, 8:38 AM

Thanks Eugene, I think the backtick issue has been addressed.

I believe all review comments have been address, except for the discussion on implementing this in the CFE or as a tidy check.

Could try firing off an RFC to cfe-dev see if other people want to weigh in.

@njames93, I'll take a crack at implementing a cfe diagnostic for this, see how far I get. Cheers :)

Looks like this can be implemented as a warning in the cfe (as @lebedev.ri suggested). I'll probably abandon this review, but will keep it active until I have the alternative cfe warning patch prepared.

I've posted https://reviews.llvm.org/D85097 to replace this review. https://reviews.llvm.org/D85097 implements this check in the CFE instead of as a tidy check per recommendation from @lebedev.ri . If acceptable, I'll abandon this review.