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; ---------------- 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). 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
