aaron.ballman 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)) ---------------- jfb wrote: > 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. > I don't really care. My original point was that any suggested fix should be > correct, and (1LL << 64) - 1 wasn't. Agreed with this. We can just leave the fix off in the circumstances it's not correct while still warning that the original code is suspicious. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66397/new/ https://reviews.llvm.org/D66397 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
