aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM! Please also add a release note for the fix when you land it.
================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:605
+ unless(isMacro()), unless(isInTemplateInstantiation()),
+ hasRHS(ignoringParenImpCasts(integerLiteral().bind(ConstId))))
.bind(OverloadId);
----------------
fwolff wrote:
> 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.
> Yes, it is hard to see, but here the constant is actually read.
Oh, thanks for the info!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122111/new/
https://reviews.llvm.org/D122111
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
