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