alexeyr marked an inline comment as done. alexeyr added inline comments.
================ Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp:114 if (P.a[X++] != P.a[X++]) return 1; + if (X && X++ && X) return 1; ---------------- alexeyr wrote: > aaron.ballman wrote: > > alexeyr wrote: > > > aaron.ballman wrote: > > > > What do you think about the following? > > > > ``` > > > > bool foo(int&); > > > > bool bar(); > > > > > > > > int i; > > > > if (foo(i) && bar() && foo(i)) return 1; > > > > ``` > > > > I think that this should not be warned on (under the assumption that > > > > the reference variable can be modified by the call and thus may or may > > > > not be duplicate), but didn't see a test covering it. > > > > > > > > It also brings up an interesting question about what to do if those > > > > were non-const pointers rather than references, because the data being > > > > pointed to could be modified as well. > > > > > > > > (If you think this should be done separately from this review, that's > > > > totally fine by me, it looks like it would be an issue with the > > > > original code as well.) > > > > > > > > One thing that is missing from this review are tests for the overloaded > > > > operator functionality. > > > This is actually handled correctly, by the same logic as `(X && X++ && > > > X)`, so I don't think it needs a separate test. The drawback is that: > > > > > > 1. it's too conservative, `X && bar() && X` isn't warned on either, > > > because I don't know a way to check that `bar()` doesn't have side > > > effects //on `X`// and so just test `HasSideEffects` > > > (https://stackoverflow.com/questions/60035219/check-which-variables-can-be-side-effected-by-expression-evaluation-in-clang-ast). > > > > > > 2. the original code does have the same issue and I didn't fix it, so > > > `foo(X) && foo(X)` and `X++ && X++` do get a warning. > > > > > > I'll add overloaded operator tests. > > Okay, that seems reasonable to me, thank you! > I've added the tests (which uncovered a problem not limited to overloaded > operators; I needed to skip uninteresting nodes when looking at parents as > well). Do you have any insight on https://reviews.llvm.org/D73775#1851553? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73775/new/ https://reviews.llvm.org/D73775 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
