[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>(
----------------
baloghadamsoftware wrote:
> aaron.ballman wrote:
> > I think this check should probably live in the `bugprone` module, WDYT?
> Based on my experience, `bugpronbe` is for checks whose findings are bugs that lead to undefined illegal memory access, behavior etc. This one is somewhere between that and readability. For example, `redundant-expression` is also in `misc`. But if you wish, I can move this checker into `bugprone`.
The `bugprone` module has less to do with memory access or undefined behavior specifically and more to do with checks that should expose bugs in your code but don't belong to other categories. We try to keep checks out of `misc` as much as possible these days and this code pattern is attempting to find cases where the user potentially has a bug, so I think `bugprone` is the correct home for it.

However, `bugprone` has a similar check and I sort of wonder whether we should be extending that check rather than adding a separate one. See `bugprone-branch-clone` which catches the highly related situation where you have a chain of conditionals and one of the conditions is repeated. e.g.,
```
if (foo) {
  if (foo) { // Caught by misc-redundant-condition
  }
} else if (foo) { // Caught by bugprone-branch-clone
}
```
Even if we don't combine the checks, we should ensure their behaviors work well together (catch the same scenarios, don't repeat diagnostics, etc).


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-redundant-condition.cpp:1097
+  }
+}
----------------
baloghadamsoftware wrote:
> aaron.ballman wrote:
> > 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.
> The check currently misses these cases completely, so I left a `FIXME` there. I can do it in the near future, but first I would like to commit a first version so I can tell the user who requested it that his request is fulfilled.
Sounds great to me, thanks!


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

https://reviews.llvm.org/D81272