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:
> > > > > 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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81272/new/
https://reviews.llvm.org/D81272
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
