aaron.ballman added a comment.
Poking @alexfh for more opinions about check similarity.
================
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:
> > > > 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).
> There are many checks that users almost always keep enabled together, but
> they are still separate checks. Now I looked into the branch clone check,
> combining them means simply copying them together because the logic is so
> much different.
>
> Even from the user's perspective I see that branches with identical
> conditions are different from redundant checks. While the first one is a more
> serious bug (the second branch with the same condition is never executed)
> this one is slightly more than a readability error.
> There are many checks that users almost always keep enabled together, but
> they are still separate checks.
I cannot find an instance with two checks that are this strongly related. The
closest I can come are some of the C++ Core Guideline checks, but those are a
different beast because they're part of a set of guidelines.
> Now I looked into the branch clone check, combining them means simply copying
> them together because the logic is so much different.
This is not a very compelling reason to make a decision to split the checks, to
me. We have plenty of checks with complex matchers and checking logic.
> Even from the user's perspective I see that branches with identical
> conditions are different from redundant checks. While the first one is a more
> serious bug (the second branch with the same condition is never executed)
> this one is slightly more than a readability error.
I don't view the proposed check as having anything to do with readability.
Readability is "how do I make the code do the same thing but look prettier?"
and other stylistic choices. This check is finding a case where the programmer
has potentially made a logical mistake with their code and is considerably more
serious than a matter of style. To me, these are identical problems of
programmer confusion.
The more I consider this, the more strongly I feel about combining the checks.
I would have a hard time understanding why this code should require two
different checks to be enabled to catch what amounts to the same logical
confusion:
```
if (!foo) {
} else if (foo) { // This is a chain of conditionals with a redundant check
}
if (!foo) {
} else {
if (foo) { // This is not a chain of conditionals, but it still has a
redundant check
}
}
```
@alexfh do you have thoughts on this?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81272/new/
https://reviews.llvm.org/D81272
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
