[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:
> > 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).
> OK, I will put this into `bugprone`. The two checks may look similar, but this one is more complex because it does not check for the same condition in multiple branches of the same branch statement but checks whether the condition expression could be mutated between the two branch statements. Therefore the the whole logic is totally different, I see no point in merging the two. Should I create a test case then, where both are enabled?
> Therefore the the whole logic is totally different, I see no point in merging the two. 

I'm approaching the question from the perspective of the user, not a check author. These two checks do the same thing (find redundant conditions in flow control which look like they could be a logical mistake), so why should they be two separate checks? "Because the code looks different" isn't super compelling from that perspective, so I'm trying to figure out what the underlying principles are for the checks. If they're the same principle, they should be the same check. If they're fundamentally different principles, we should be able to explain when to use each check as part of their documentation without it sounding contrived. (Note, I'm not saying the checks have to be combined, but I am pushing back on adding an entirely new check that seems to be redundant from a user perspective.)

As a litmus test: can you think of a situation where you'd want only one of these two checks enabled? I can't think of a case where I'd care about redundancy in nested conditionals but not in chained conditionals (or vice versa) unless one of the checks had a really high false positive rate (which isn't much of a reason to split the checks anyway).

> Should I create a test case then, where both are enabled?

If we wind up keeping the checks separate, then probably yes (also, the documentation for the checks should be updated to explain how they're different and why that's useful).


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

https://reviews.llvm.org/D81272