[PATCH] D81272: [clang-tidy] New check `misc-redundant-condition`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp:46
+    CheckFactories.registerCheck<RedundantConditionCheck>(
+        "misc-redundant-condition");
     CheckFactories.registerCheck<RedundantExpressionCheck>(
----------------
I think this check should probably live in the `bugprone` module, WDYT?


================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:32
+  const auto &SM = Context->getSourceManager();
+  if (const Stmt *MutS = MutAn.findMutation(Var)) {
+    if (SM.isBeforeInTranslationUnit(MutS->getEndLoc(), NextS->getBeginLoc()))
----------------
This code can be simplified into:
```
const Stmt *MutS = MutAn.findMutation(Var);
return MutS && SM.isBeforeInTranslationUnit(MutS->getEndLoc(), NextS->getBeginLoc());
```


================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:73
+  // If the variable has an alias then it can be changed by that alias as well.
+  // FIXME: Track pointers and references.
+  if (hasPtrOrReferenceInFunc(Func, CondVar))
----------------
This FIXME makes me worried that what we really want is a clang static analyzer check, since the check is already flow sensitive and needs to be data sensitive as well. Have you considered implementing this as a static analyzer check?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-redundant-condition.cpp:1097
+  }
+}
----------------
Can you add some tests that exercise GNU expression statements, just to make sure we get the behavior correct?
```
if (({foo;})) {
} else if (({foo;})) {
}

if (foo) ({bar;});
else if (foo) ({bar;});
```
Another thing that's interesting to test is whether the redundant expression is in a subexpression which doesn't contribute to the value of the control expression:
```
if (foo(), val) {
} else if (foo(), other_val) {
}
```
(Similar can be tested with GNU statement expressions.)


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

https://reviews.llvm.org/D81272