baloghadamsoftware marked 3 inline comments as done.
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:
> 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`.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-condition.cpp:1097
+ }
+}
----------------
aaron.ballman wrote:
> baloghadamsoftware wrote:
> > aaron.ballman wrote:
> > > Can you add some tests that exercise GNU expression statements, just to
> > > make sure we get the behavior correct?
> > > ```
> > > if (({foo;})) {
> > > } else if (({foo;})) {
> > > }
> > >
> > > if (foo) ({bar;});
> > > else if (foo) ({bar;});
> > > ```
> > > Another thing that's interesting to test is whether the redundant
> > > expression is in a subexpression which doesn't contribute to the value of
> > > the control expression:
> > > ```
> > > if (foo(), val) {
> > > } else if (foo(), other_val) {
> > > }
> > > ```
> > > (Similar can be tested with GNU statement expressions.)
> > Do you mean that I should handle these cases as well? (Detect a bug,
> > provide a fix.)
> If the check misses these cases, I think that's reasonable (just comment that
> the tests aren't currently handled). If the check diagnoses these cases but
> the fixit causes an issue, then I think that should be corrected in this
> patch. If it's trivial to support these cases (diagnosing or fixing), it's
> your call on whether you want to do so. I mostly just want the test coverage
> so I can know what to expect.
The check currently misses these cases completely, so I left a `FIXME` there. I
can do it in the near future, but first I would like to commit a first version
so I can tell the user who requested it that his request is fulfilled.
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
