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