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

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

baloghadamsoftware marked 3 inline comments as done.
baloghadamsoftware added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp:46
+    CheckFactories.registerCheck<RedundantConditionCheck>(
+        "misc-redundant-condition");
     CheckFactories.registerCheck<RedundantExpressionCheck>(
----------------
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`.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-redundant-condition.cpp:1097
+  }
+}
----------------
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.


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

https://reviews.llvm.org/D81272