aaron.ballman added a comment.
Thanks to the new info, I think the check basically LGTM. Can you add some
negative tests and documentation wording to make it clear that the check
doesn't currently handle all logically equivalent predicates, like:
if (foo) {
} else {
if (!foo) {
}
}
// or
if (foo > 5) {
if (foo > 3) {
}
}
// or
if (foo > 5) {
if (5 < foo) {
}
}
(I'm assuming these cases aren't handled currently and that handling them isn't
necessary to land the patch.)
================
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:
> > > > > > 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?
> I almost started to copy the two checks together when one of my colleagues
> told me that `bugprone-branch-clone` is not at all about redundant conditions
> but redundant bodies in different branches. (It was created by a formal
> student intern of our team.) Thus even from the user's perspective these
> checkers have nothing to do with each other.
Whoa, I had not noticed that about `bugprone-branch-clone`, but I reread the
docs and you're absolutely right. Great catch and I'm sorry for the accidental
noise.
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
