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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
