aaron.ballman 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: > > 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! 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
