[PATCH] D84898: [clang-tidy] Add new checker for complex conditions with no meaning

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

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



================
Comment at: clang-tools-extra/clang-tidy/misc/ComplexConditionsCheck.cpp:28-33
+  const auto *If = Result.Nodes.getNodeAs<IfStmt>("complex_binop");
+  const auto *Statement = Result.Nodes.getNodeAs<Stmt>("complex_binop");
+  if (If) {
+    diag(If->getBeginLoc(),
+         "comparisons like `X<=Y<=Z` have no mathematical meaning");
+  }
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/misc/ComplexConditionsCheck.cpp:34
+  }
+  if (Statement) {
+    diag(Statement->getBeginLoc(),
----------------
Elide braces


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84898/new/

https://reviews.llvm.org/D84898