[PATCH] D66397: [Diagnostics] Improve -Wxor-used-as-pow

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.


================
Comment at: lib/Sema/SemaExpr.cpp:11067
+
+  // Do not diagnose 2 ^ 64, but allow special case (2 ^ 64) - 1.
+  if (SubLHS && SubRHS && (LeftSideValue != 2 || RightSideValue != 64))
----------------
xbolva00 wrote:
> xbolva00 wrote:
> > aaron.ballman wrote:
> > > xbolva00 wrote:
> > > > xbolva00 wrote:
> > > > > aaron.ballman wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > This comment explains what the code does, but not why it does it. Given that we're adding special cases, I think more comments here explaining why this is valuable would be appreciated.
> > > > > > Thank you, the comments helped! But they also raised another question for me. What's special about 2^64? Why not (2^16) - 1 or other common power-of-two values? I would have expected 8, 16, 32, and 64 to be handled similarly.
> > > > > We generally suggest 1LL << C. But here we cant say 1LL << 64.
> > > > > 
> > > > > (2^16)-1 is diagnosed normally since we go here from “visit xor” code.
> > > > >  ^^^^
> > > > > 
> > > > > This (2^64)-1 handling was suggested by jfb.
> > > > > 
> > > > > I see no motivation cases to diagnose 2^65, 2^100, ...
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > I think the suggestion for (2^32)-1:
> > > > (1LL<32)-1 is good, or should we make things more complicated and suggest Int max macro? 
> > > > We generally suggest 1LL << C. But here we cant say 1LL << 64.
> > > 
> > > Ah, good point.
> > > 
> > > > I see no motivation cases to diagnose 2^65, 2^100, ...
> > > 
> > > Me neither, I was more wondering about the common powers of two.
> > > 
> > > > I think the suggestion for (2^32)-1:
> > > > (1LL<32)-1 is good, or should we make things more complicated and suggest Int max macro?
> > > 
> > > I feel like this is somewhat clang-tidy territory more than the compiler properly, including the 2^64 - 1 case, because it is likely to be very uncommon due to how specific it is. However, given that this only triggers on xor and only with integer literals, it shouldn't cause too much of a compilation slow-down in general to do it here.
> > > 
> > > I tend to err on the side of consistency, so my feeling is that if we want the 64 case to suggest ULLONG, we'd want the other cases to behave similarly. Alternatively, rather than handling this specific issue in the compiler, handle it in a `bugprone` clang-tidy check where we can also give the user more control over how they want to correct their mistake (e.g., `std::numeric_limits<long>::max()` vs `LONG_MAX` vs `~0L`).
> > @jfb ?
> (There were no concerns about slowdown in the “base” patch).
> 
> Maybe I should just revert it..
I don't really care. My original point was that any suggested fix should be correct, and `(1LL << 64) - 1` wasn't.


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

https://reviews.llvm.org/D66397