[PATCH] D73775: [clang-tidy] Cover cases like (b && c && b) in the redundant expression check

Mon Feb 3 10:12:00 PST 2020

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