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

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.


================
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))
----------------
aaron.ballman wrote:
> 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?
Actually, it is only a proposal for improvement. Maybe FIXME is not the best wording for it. This is not a bug to fix, just a possibility to find more true positives. This check is very similar to the infinite loop check which is also in Tidy. In the Static Analyzer this check would be too heavyweight and would probably produce false positives.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-redundant-condition.cpp:1097
+  }
+}
----------------
aaron.ballman wrote:
> 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.)
Do you mean that I should handle these cases as well? (Detect a bug, provide a fix.)


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

https://reviews.llvm.org/D81272