[PATCH] D122111: [clang-tidy] Fix false positives in `misc-redundant-expression` check

[Fabian Wolff via Phabricator via cfe-commits]

fwolff marked an inline comment as done.
fwolff added a comment.

Thanks for the quick review @aaron.ballman! I think I've addressed your comments.



================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:605
+          unless(isMacro()), unless(isInTemplateInstantiation()),
+          hasRHS(ignoringParenImpCasts(integerLiteral().bind(ConstId))))
           .bind(OverloadId);
----------------
aaron.ballman wrote:
> This seems incorrect to me -- the LHS and RHS can be swapped along with which operator is used and still achieve the same semantic results.
> 
> While playing around to test the behavior we currently have, I was a bit surprised at the difference in results here: https://godbolt.org/z/hE4qfca1b.
> 
> Also, do we need to bind? Nothing seems to look at that binding that I can see.
> This seems incorrect to me -- the LHS and RHS can be swapped along with which operator is used and still achieve the same semantic results.

Fixed.

> While playing around to test the behavior we currently have, I was a bit surprised at the difference in results here: https://godbolt.org/z/hE4qfca1b.

All four conditions get a warning with my changes. I've added your example to the test file.

> Also, do we need to bind? Nothing seems to look at that binding that I can see.

Yes, it is hard to see, but [[ https://github.com/llvm/llvm-project/blob/240e06dfe77feabe38a03cbb1c94875639d0f9ff/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp#L498 | here ]] the constant is actually read.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122111/new/

https://reviews.llvm.org/D122111