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

[Alexey Romanov via Phabricator via cfe-commits]

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