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:
> 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.
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
