[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/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))
----------------
baloghadamsoftware wrote:
> 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.
Ah, thank you for clarifying (though I think it's interesting that we come down on opposite sides of the FP question -- I would argue that the static analyzer check would have considerably fewer false positives than the tidy check when handling this case).

If the idea is that pointers and references need to be supported in a future patch, I think FIXME is a fine comment. If the idea is that pointers and references are a potential improvement, I'd probably clarify the comment to something more like `FIXME: could potentially support tracking pointers and references in the future to improve catching true positives through aliases.`


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-redundant-condition.cpp:1097
+  }
+}
----------------
baloghadamsoftware wrote:
> 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.)
If the check misses these cases, I think that's reasonable (just comment that the tests aren't currently handled). If the check diagnoses these cases but the fixit causes an issue, then I think that should be corrected in this patch. If it's trivial to support these cases (diagnosing or fixing), it's your call on whether you want to do so. I mostly just want the test coverage so I can know what to expect.


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

https://reviews.llvm.org/D81272