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: > 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. Repository: rG LLVM Github Monorepo 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
