[Diagnostics] Improve -Wxor-used-as-pow

[xbolva00]

Summary

Handle specific cases

(2 ^ 64) - 1
Digit separators

[xbolva00]

From post commit discussion, Nico Weber:

1. Hi,
3. this results in a false positive on webrtc, on this code:
5. https://cs.chromium.org/chromium/src/third_party/libwebp/src/enc/picture_csp_enc.c?q=picture_csp_enc.c&sq=package:chromium&dr&l=1002
7. The code does this:
9. #ifdef WORDS_BIGENDIAN
10. #define ALPHA_OFFSET 0 // uint32_t 0xff000000 is 0xff,00,00,00 in memory
11. #else
12. #define ALPHA_OFFSET 3 // uint32_t 0xff000000 is 0x00,00,00,ff in memory
13. #endif
15. // ...
17. const uint8_t* const argb = (const uint8_t*)picture->argb;
18. const uint8_t* const a = argb + (0 ^ ALPHA_OFFSET);
19. const uint8_t* const r = argb + (1 ^ ALPHA_OFFSET);
20. const uint8_t* const g = argb + (2 ^ ALPHA_OFFSET);
21. const uint8_t* const b = argb + (3 ^ ALPHA_OFFSET);
23. The idea is to get bytes 0, 1, 2, 3 as a, r, g, b if ALPHA_OFFSET is 0, or bytes 3, 2, 1, 0 if ALPHA_OFFSET is 3.
25. Maybe this shouldn't fire if the rhs is a macro?
27. (On all of Chromium, this fired 3 times; in the other 2 cases the rhs was a literal as well, and those two were true positives.)

[xbolva00]

I agree what @tkanis suggested and be silent if RHS is macro as real world code shows it. Opinions?

@jfb @aaron.ballman

[xbolva00]

Do not warn if RHS is macro.

[aaron.ballman]

In D66397#1635715, @xbolva00 wrote:

I agree what @tkanis suggested and be silent if RHS is macro as real world code shows it. Opinions?

@jfb @aaron.ballman

I suspect we can come up with examples where the macro on either lhs or rhs is sensible and other examples where its senseless. The existing test cases already have:

res = TWO ^ 8; // expected-warning {{result of 'TWO ^ 8' is 10; did you mean '1 << 8' (256)?}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:16}:"1 << 8"
// expected-note@-2 {{replace expression with '0x2 ^ 8' to silence this warning}}

res = 2 ^ TEN; // expected-warning {{result of '2 ^ TEN' is 8; did you mean '1 << TEN' (1024)?}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:16}:"1 << TEN"
// expected-note@-2 {{replace expression with '0x2 ^ TEN' to silence this warning}}

showing that we expect to warn when macros are involved. If we decide that macros should silence the warning, I would expect any use of a macro in the expression to silence it, not just a RHS macro.

[xbolva00]

I think it is too soon to jump and disable all macros. We still catch all motivating cases, it found bugs in Chromium.

Chromium's case makes sense but on the other hand, LHS does not make much sense to be a macro. #define TWO 2 and res = TWO ^ 32? And even for this case, I think the user should rework it to #define TWO 0x2.

[aaron.ballman]

In D66397#1635745, @xbolva00 wrote:

I think it is too soon to jump and disable all macros. We still catch all motivating cases, it found bugs in Chromium.

I think it's too soon to disable any macros. The workaround for Chromium is trivial and makes the code more clear.

Chromium's case makes sense but on the other hand, LHS does not make much sense to be a macro. #define TWO 2 and res = TWO ^ 32? And even for this case, I think the user should rework it to #define TWO 0x2.

I guess I don't see the distinction. If the user should rework TWO to be 0x2 when on the LHS, the same holds true for the RHS, to me.

[xbolva00]

I think it's too soon to disable any macros.

It is hard to say (this was discussed a lot).

@thakis or @hans what do you think? Maybe you could do experiments for us. Comment the code which disables this warning in macros and try it on Chromium - let’s see how many false positives/negatives it could produce. This could really resolve “macro” question.

But please let’s not repeat same “macro yes/no” arguments from last review.

Anyway, I could make -Wxor-used-as-pow-in-macro so everybody would be happy. Is it solution?

Now, Chromium disabled this warning :( until we “fix” it.

[• Quuxplusone]

In D66397#1635796, @xbolva00 wrote:

Now, Chromium disabled this warning :( until we “fix” it.

If Chromium is willing to make build-system changes to add -Wno-xor-as-pow, then surely they should also be willing to make the suggested source-code fixit of #define ALPHA_OFFSET 0x3 where hexadecimal indicates an intentional bitwise operation. Let's give them a little time to react to that suggestion. :)
(A heavier workaround would be to use xor instead of ^.)

[xbolva00]

>> suggested source-code fixit of #define ALPHA_OFFSET 0x3

No, there is a note “replace with 0x2 ^ ALPHA_OFFSET” to silence it. Didnt you suggest 0x2 / 0xA? :)

While we can suggest as you wrote:
#define ALPHA_OFFSET 0x3

I really dont think it is worth to do.

My suggestion: comment macro bailout, run this on Chromium.

[xbolva00]

Any objections to this patch except “macro” question?

[riccibruno]

Is the test up-to-date ? I tried to apply the patch locally and I get a bunch of failures :

error: 'warning' diagnostics expected but not seen: 
  File /home/bruno/software/llvm-project/clang/test/SemaCXX/warn-xor-as-pow.cpp Line 73: result of '2 ^ 64' is 66; did you mean '-1LL'?
  File /home/bruno/software/llvm-project/clang/test/SemaCXX/warn-xor-as-pow.cpp Line 77: result of '2 ^ 64' is 66; did you mean 'ULLONG_MAX'?
error: 'warning' diagnostics seen but not expected: 
  File /home/bruno/software/llvm-project/clang/test/SemaCXX/warn-xor-as-pow.cpp Line 53: result of '2 ^ TEN' is 8; did you mean '1 << TEN' (1024)?
  File /home/bruno/software/llvm-project/clang/test/SemaCXX/warn-xor-as-pow.cpp Line 119: result of '2 ^ ALPHA_OFFSET' is 1; did you mean '1 << ALPHA_OFFSET' (8)?
error: 'note' diagnostics expected but not seen: 
  File /home/bruno/software/llvm-project/clang/test/SemaCXX/warn-xor-as-pow.cpp Line 73 (directive at /home/bruno/software/llvm-project/clang/test/SemaCXX/warn-xor-as-pow.cpp:75): replace expression with '0x2 ^ 64' to silence this warning
  File /home/bruno/software/llvm-project/clang/test/SemaCXX/warn-xor-as-pow.cpp Line 77 (directive at /home/bruno/software/llvm-project/clang/test/SemaCXX/warn-xor-as-pow.cpp:79): replace expression with '0x2 ^ 64' to silence this warning
error: 'note' diagnostics seen but not expected: 
  File /home/bruno/software/llvm-project/clang/test/SemaCXX/warn-xor-as-pow.cpp Line 53: replace expression with '0x2 ^ TEN' to silence this warning
  File /home/bruno/software/llvm-project/clang/test/SemaCXX/warn-xor-as-pow.cpp Line 119: replace expression with '0x2 ^ ALPHA_OFFSET' to silence this warning
8 errors generated.

lib/Sema/SemaExpr.cpp

9429

I think it would be nice to have a little comment above diagnoseXorMisusedAsPow to explain what the parameters are (for example. it is not immediately clear to me what is the difference between SubLHS and LHS).

9434

s/is macro/is a macro ?

[xbolva00]

File /home/bruno/software/llvm-project/clang/test/SemaCXX/warn-xor-as-pow.cpp Line 119: result of '2 ^ ALPHA_OFFSET' is 1; did you mean '1 << ALPHA_OFFSET' (8)?

This should not warn. Please verify that patch was applied correctly and you use newly built clang with this patch. (I use arc patch DXXXXX)

[aaron.ballman]

In D66397#1635857, @xbolva00 wrote:

>> suggested source-code fixit of #define ALPHA_OFFSET 0x3

No, there is a note “replace with 0x2 ^ ALPHA_OFFSET” to silence it. Didnt you suggest 0x2 / 0xA? :)

While we can suggest as you wrote:
#define ALPHA_OFFSET 0x3

I really dont think it is worth to do.

I don't see this as a false positive; I think Chromium should change their macro rather than Clang disable this check for everyone using macros.

[xbolva00]

I agree. And I think they should just use const int instead of macros after all.

Maybe we should just improve silence hint if rhs is macro?
#define ALPHA_OFFSET 0x3 ?

[aaron.ballman]

In D66397#1639185, @xbolva00 wrote:

I agree. And I think they should just use const int instead of macros after all.

Maybe we should just improve silence hint if rhs is macro?
#define ALPHA_OFFSET 0x3 ?

I don't think we should try to suggest a fixit to fix the macro definition because the replacement list can be arbitrarily complex. We might be able to do something for a simple object-like macro whose replacement list is a single integer literal, but I'm not certain it's worth the effort (we don't go to those lengths with fixits elsewhere, that I'm aware of).

[xbolva00]

Fixed review nits.

[xbolva00]

I fixed and enabled this checker to warn for macros too.

@aaron.ballman is right. Yes, as he said... If the user has a time to add -Wno-xor.... to build systems, he/she has also time to improve code readability and use hexadecimal literals.

But I am a bit worried that the user does not realize that he/she could fix it using hexadecimal literals... Should this be part of silence note? (any proposed wording?) or add to Clang docs?

[aaron.ballman]

In D66397#1639376, @xbolva00 wrote:

I fixed and enabled this checker to warn for macros too.

@aaron.ballman is right. Yes, as he said... If the user has a time to add -Wno-xor.... to build systems, he/she has also time to improve code readability and use hexadecimal literals.

But I am a bit worried that the user does not realize that he/she could fix it using hexadecimal literals... Should this be part of silence note? (any proposed wording?) or add to Clang docs?

That's a reasonable concern, but I'm not certain it's worth adding the text to the note for it. This seems more like a documentation issue. We have some automation for generating documentation for warning flags (see ClangDiagnosticsEmitter.cpp), and it looks like there may be a way to have the diagnostic itself provide the documentation so that it appears in https://clang.llvm.org/docs/DiagnosticsReference.html automatically. To me, that would be the best place for this documentation to live, but I have no idea if there's further work needed to support that.

[xbolva00]

But it seems everything is autogenerated and nothing is “custom message”. Any custom change is removed by next autogeneration.

Possibly @hans could tell us if there is a way

[aaron.ballman]

In D66397#1639590, @xbolva00 wrote:

But it seems everything is autogenerated and nothing is “custom message”. Any custom change is removed by next autogeneration.

Possibly @hans could tell us if there is a way

The part I saw that looked interesting to me was: https://github.com/llvm-mirror/clang/blob/master/utils/TableGen/ClangDiagnosticsEmitter.cpp#L1786 but I see now that's only for diagnostic groups. You may need to extend this functionality to get the behavior we want, but that can always be done in a follow-up patch which also adds the documentation to the diagnostic flag.

[thakis]

In D66397#1635792, @aaron.ballman wrote:

In D66397#1635745, @xbolva00 wrote:

I think it is too soon to jump and disable all macros. We still catch all motivating cases, it found bugs in Chromium.

I think it's too soon to disable any macros. The workaround for Chromium is trivial and makes the code more clear.

I think I missed the workaround. I only saw the suggestion to write 0x2 instead of 2 which doesn't seem more clear at all to me. Was there another fix suggestion?

[aaron.ballman]

In D66397#1639696, @thakis wrote:

In D66397#1635792, @aaron.ballman wrote:

In D66397#1635745, @xbolva00 wrote:

I think it is too soon to jump and disable all macros. We still catch all motivating cases, it found bugs in Chromium.

I think it's too soon to disable any macros. The workaround for Chromium is trivial and makes the code more clear.

I think I missed the workaround. I only saw the suggestion to write 0x2 instead of 2 which doesn't seem more clear at all to me. Was there another fix suggestion?

Nope, you didn't miss it -- that was the suggestion. Using a hex notation is the way we let programmers signify that they want to do bit fiddling.

[lebedev.ri]

I also strongly -1 to disabling the warning for macros.
It's just a disservice to everyone, and the fact there's precedent doesn't mean it's the right solution.

That being said, does using C++14 binary literals also silence the diag? It should.

[thakis]

I think I missed the workaround. I only saw the suggestion to write 0x2 instead of 2 which doesn't seem more clear at all to me. Was there another fix suggestion?

Nope, you didn't miss it -- that was the suggestion. Using a hex notation is the way we let programmers signify that they want to do bit fiddling.

Sorry, I don't buy that :) If I wasn't aware it's there to suppress a warning, if I saw 0x2 I'd think the author of that was confused about what the 0x prefix does. Maybe a better fixit is to suggest xor instead of ^ which also suppresses the warning and which is imho a bit less weird. (xor is so rare that it's still a bit weird though.)

[xbolva00]

C++14 binary literals

Thanks, noted.

Suggest “xor”

We could. Aaron what do you think? But I still like “0x” more.

[aaron.ballman]

In D66397#1639749, @thakis wrote:

I think I missed the workaround. I only saw the suggestion to write 0x2 instead of 2 which doesn't seem more clear at all to me. Was there another fix suggestion?

Nope, you didn't miss it -- that was the suggestion. Using a hex notation is the way we let programmers signify that they want to do bit fiddling.

Sorry, I don't buy that :) If I wasn't aware it's there to suppress a warning, if I saw 0x2 I'd think the author of that was confused about what the 0x prefix does.

I don't understand how you draw that conclusion, but the reason behind why we went with that as a way to silence the diagnostic is because using the prefix acts as a signal that the developer wants to do bit manipulation more than just a decimal literal does. It's not ideal because it's largely a hidden way to silence diagnostics, but it's also not the only place where we do that sort of thing (like surrounding something in parens to silence a diagnostic, or casting something to void, etc).

Maybe a better fixit is to suggest xor instead of ^ which also suppresses the warning and which is imho a bit less weird. (xor is so rare that it's still a bit weird though.)

I don't think suggesting a fixit for xor is a good solution. For starters, you need a header included in order to use that for C. As a textual suggestion "; use the 'xor' alternative token to perform an exclusive OR" wouldn't bother me too much though.

[xbolva00]

Could the following text be acceptable?

replace expression with '0xA ^ 1' or ‘10 xor 1’ to silence this warning

(In C mode without xor macro “replace expression with '0xA ^ 1' to silence this warning”)

I am not fan of “the ‘xor’ alternative token”

[rsmith]

The ALPHA_OFFSET code seems to be unnecessarily "clever" to me. I think the warning can be suppressed by reversing the operands:

ALPHA_OFFSET ^ 2

But if I were maintaining that code I would get rid of the xor hack entirely and use

#define CHANNEL_OFFSET(i) (i)

or

#define CHANNEL_OFFSET(i) (3-(i))

[xbolva00]

Swap trick is cool, we can suggest it always(vs. ‘xor’)

Or maybe not :) 2 ^ 2 :)

[• Quuxplusone]

In D66397#1639840, @xbolva00 wrote:

Swap trick is cool, we can suggest it always(vs. ‘xor’)

Well, you should avoid suggesting that the user rewrite 2 ^ 10 as 10 ^ 2.
Anyway, I think some of the observers here might be confusing two aspects of the issue.

First there's "how can the programmer shut up this diagnostic?", to which there are dozens of answers that all work equally well. 0x2 ^ 3, 2 ^ 0x3, 2 xor 3, 3 ^ 2 (in this case), myxorfunction(2, 3), 02 ^ 03, 0b10 ^ 0b11, Richard's suggested de-cleverfication of 3 - 2, and so on. Basically anything that convinces the compiler you're not trying to take an integer power of 2.

Second there's "what advice and/or fixit should Clang produce to push the programmer toward one specific way of shutting up this diagnostic?". Right now, IIUC, Clang always gives an English note suggesting to change the left-hand side into hex (preserving the behavior of the code), and a technical fixit suggesting to use 1 << RHS or 1eRHS (changing the behavior of the code).

The main trouble we're having with #2 is that when Clang suggests one possible fix, the Chromium programmers seemed to interpret that as meaning that that was the only possible fix — they didn't look deeper to see if there might be other possible ways to improve the code.

Perhaps you could change the English note to say

to silence this warning, replace expression with '0x2 ^ ALPHA_OFFSET' or use the 'xor' keyword

This would at least give more of a jog to the programmer's brain, by saying "you have a choice to make here; don't apply the compiler's suggested fixit blindly."

[xbolva00]

Detect digit separators.

[xbolva00]

Implemented @Quuxplusone's idea to improve silence note.

[xbolva00]

I think everybody should be fine with this now :)

[thakis]

I don't understand how you draw that conclusion, but the reason behind why we went with that as a way to silence the diagnostic is because using the prefix acts as a signal that the developer wants to do bit manipulation more than just a decimal literal does. It's not ideal because it's largely a hidden way to silence diagnostics, but it's also not the only place where we do that sort of thing (like surrounding something in parens to silence a diagnostic, or casting something to void, etc).

I think adding parens and casts are fairly well-understood to suppress warnings. Other things, like changing the spelling of literals, aren't. C++ is already a fairly baroque language, and every new thing that makes your code behave subtly different with a different compiler makes it more so. So I think we should be careful to not attach semantic meaning in surprising ways. Warnings have to carry their weight.

Maybe a better fixit is to suggest xor instead of ^ which also suppresses the warning and which is imho a bit less weird. (xor is so rare that it's still a bit weird though.)

I don't think suggesting a fixit for xor is a good solution. For starters, you need a header included in order to use that for C. As a textual suggestion "; use the 'xor' alternative token to perform an exclusive OR" wouldn't bother me too much though.

I don't like xor all that much either :)

I do like Richard's suggestion.

Quuxplusone: Saying "the chromium devs don't want to change their code" is missing the mark. I'm happy to change that file, but that doesn't really help. There are hundreds of people committing hundreds of changes to the code base every day. They have varying levels of C++ proficiency (like everywhere). It's not just about getting our code to compile today, but also about the long-term effects of the warnings we have enabled. We have to be careful about which warnings we enable, and this one is maybe not quite there yet on a usefulness/overhead tradeoff evaluation. I'd argue that Chromium is fairly typical here and that clang's default warning set should follow similar considerations.

I looked through D63423 and didn't find an evaluation of false / true positive rates when lhs and rhs are just literals vs when they're identifiers / macros. Glancing through https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=10+%5E&search=Search , I couldn't find a single true positive where lhs or rhs weren't a bare literal (but I didn't look super carefully). Did I just miss that in D63423? What was the motivation for firing on more than bare literals?

[xbolva00]

I think adding parens and casts are fairly well-understood to suppress warnings.

It should work here as well. #define ALPHA_OFFSET (3). Did anobody from Chromium try it?

They have varying levels of C++ proficiency

I consider things like hex decimals or parens to silence as a quite basic knowledge.

We have to be careful about which warnings we enable

I dont want off by default warning - useless. If there are still infinite macro concerns, I will revert just base patch.

[riccibruno]

This should not warn. Please verify that patch was applied correctly and you use newly built clang with this patch. (I use arc patch DXXXXX)

You were right, I messed up on my side. Sorry about the noise !

I don't have much to add to the macro yes/no discussion but I have added some inline comments.

lib/Sema/SemaExpr.cpp

9449

This will miss literals with a unary +, like 10 ^ +8. I am not finding any code sample on codesearch.isocpp.org with this pattern, but you could imagine someone writing code like :

#define EXPONENT 10

res1 = 10 ^ +EXPONENT;
res2 = 10 ^ -EXPONENT;

WDYT ?

9469

You could bailout early if the values are such that no diagnostics is ever going to be issued, before doing any source range/string manipulations (ie: do all the tests on the values first).

9506

Maybe put the slightly more expensive find last in the condition.

9670

I think that this will miss code like (2 ^ 64) - 1u since IgnoreParens() will not skip any implicit casts added by UsualArithmeticConversions. (Also IgnoreParens() skips a bunch of other things, but it does not seem relevant here).

Also, in CheckBitwiseOperands diagnoseXorMisusedAsPow is called before UsualArithmeticConversions and not after. I am wondering if it is possible for diagnoseXorMisusedAsPow to be called with invalid arguments in CheckBitwiseOperands ?

[thakis]

In D66397#1640685, @xbolva00 wrote:

I think adding parens and casts are fairly well-understood to suppress warnings.

It should work here as well. #define ALPHA_OFFSET (3). Did anobody from Chromium try it?

They have varying levels of C++ proficiency

I consider things like hex decimals or parens to silence as a quite basic knowledge.

parens for sure, but I'm not aware of any other warning that behaves differently for ordinary and hex literals. What else is there?

[xbolva00]

other warning that behaves differently for ordinary and hex literals

Such literals should indicate that the user knows what he/she is doing (bit tricks, ..). I agree that there should be a additional docs (as we mentioned it and kindly pinged @hans) and not force the user to looking at Clang's code to see what are preconditions to warn.

I dont know really how to make both sides happy in terms of "macro" issue :(

[aaron.ballman]

In D66397#1639818, @rsmith wrote:

The ALPHA_OFFSET code seems to be unnecessarily "clever" to me. I think the warning can be suppressed by reversing the operands:

ALPHA_OFFSET ^ 2

But if I were maintaining that code I would get rid of the xor hack entirely and use

#define CHANNEL_OFFSET(i) (i)

or

#define CHANNEL_OFFSET(i) (3-(i))

+1

In D66397#1641121, @thakis wrote:

In D66397#1640685, @xbolva00 wrote:

I think adding parens and casts are fairly well-understood to suppress warnings.

It should work here as well. #define ALPHA_OFFSET (3). Did anobody from Chromium try it?

They have varying levels of C++ proficiency

I consider things like hex decimals or parens to silence as a quite basic knowledge.

parens for sure, but I'm not aware of any other warning that behaves differently for ordinary and hex literals. What else is there?

I thought we did in Clang proper, but now I am second-guessing because I cannot find any with a quick search. Regardless, I think the "how do we silence this diagnostic" is getting a bit out of hand. We seem to now silence it when there's a hex, oct, or binary literal, a digit separator, or the xor token. I think we do not want to list all of those options in a diagnostic message.

I looked through D63423 and didn't find an evaluation of false / true positive rates when lhs and rhs are just literals vs when they're identifiers / macros. Glancing through https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=10+%5E&search=Search , I couldn't find a single true positive where lhs or rhs weren't a bare literal (but I didn't look super carefully). Did I just miss that in D63423? What was the motivation for firing on more than bare literals?

This is an interesting question to me that may help inform how we want to proceed.

[xbolva00]

What was the motivation for firing on more than bare literals?

The first patch (or so) supported diagnosing macros. There is no exact motivation why, simply “I just did it” to present working prototype. Based on review, things were adjusted.

Some reviewers wanted to keep full macro support, some didnt like diagnose ^ in macros. That case was removed.

I think nobody complained about macro as LHS/RHS.

[xbolva00]

Addressed Bruno Ricci's tips.
Removed macro support.

[xbolva00]

Removed macro support (again...).

I agree with Nico, this warning is on by default so should be as exact as possible without many false positives. While we all know that the Chromium's false positive case could be rewritten for better code readability, on the other side Clang's default warnings shouldn't be annoying.

Nico, PTAL and possibly approve.

[• Quuxplusone]

@thakis wrote:

What was the motivation for firing on more than bare literals?

Well, fundamentally, there is no difference in code quality between any of these snippets:

#define BIG 300
double bigpower1() { return 10 ^ BIG; }

static constexpr int BIG = 300; double bigpower2() { return 10 ^ BIG; }

double bigpower3(int BIG) { return 10 ^ BIG; }

double bigpower4(int BIG) { return 10 ^ (BIG+1); }

minval -= 10 ^ -precision;  // https://codesearch.isocpp.org/actcd19/main/q/qgis/qgis_2.18.28+dfsg-2/src/gui/editorwidgets/qgsrangewidgetwrapper.cpp
intermediate = (str[offset] - '0') / (10 ^ lpc);  // https://codesearch.isocpp.org/actcd19/main/p/pacemaker/pacemaker_1.1.18-2/lib/common/iso8601.c
real_loop += (((unsigned int) *argv[4]+k) - 48) * 10^(strlen(argv[4]) - (k+1));  // https://codesearch.isocpp.org/actcd19/main/liba/libaria/libaria_2.8.0+repack-1.2/tests/testCOM.cpp

If you seriously mean "xor", then you shouldn't be writing code that looks like you meant "exponentiate." I don't think it matters whether the LHS or RHS come from literals, macros, constexpr variables, const variables, function calls, additions, subtractions, or what-have-you. It seems reasonable, when you write 10 ^ x or 2 ^ x in C++, for the compiler to tell you about it so you can go fix it. (Admittedly it looks like a slippery slope to int square(int n) { return n ^ 2; } — but we can be data-driven here. There are no true-positive instances of ^ 2 in our corpus, whereas there are many true positives of 10 ^ and 2 ^.)

To the target audience of this warning, the news about ^'s semantics would be coming as a surprise. Not every user of C++ is a power user; and only power users would be like "I know 10 ^ alpha means alpha xor 0xA and in fact that's exactly what I meant — please don't warn me about my code!"

However, if this is the version that gets committed, it's far better than nothing, and it can always be further tightened up later. (For example, this version doesn't yet warn about bigpower3 et seq., right? It basically only hits the literal cases.)

[thakis]

In D66397#1642012, @Quuxplusone wrote:

@thakis wrote:

What was the motivation for firing on more than bare literals?

Well, fundamentally, there is no difference in code quality between any of these snippets:

#define BIG 300
double bigpower1() { return 10 ^ BIG; }

static constexpr int BIG = 300; double bigpower2() { return 10 ^ BIG; }

double bigpower3(int BIG) { return 10 ^ BIG; }

double bigpower4(int BIG) { return 10 ^ (BIG+1); }

minval -= 10 ^ -precision;  // https://codesearch.isocpp.org/actcd19/main/q/qgis/qgis_2.18.28+dfsg-2/src/gui/editorwidgets/qgsrangewidgetwrapper.cpp
intermediate = (str[offset] - '0') / (10 ^ lpc);  // https://codesearch.isocpp.org/actcd19/main/p/pacemaker/pacemaker_1.1.18-2/lib/common/iso8601.c
real_loop += (((unsigned int) *argv[4]+k) - 48) * 10^(strlen(argv[4]) - (k+1));  // https://codesearch.isocpp.org/actcd19/main/liba/libaria/libaria_2.8.0+repack-1.2/tests/testCOM.cpp

Thanks for the real-world examples, these are convincing.

If you seriously mean "xor", then you shouldn't be writing code that looks like you meant "exponentiate." I don't think it matters whether the LHS or RHS come from literals, macros, constexpr variables, const variables, function calls, additions, subtractions, or what-have-you. It seems reasonable, when you write 10 ^ x or 2 ^ x in C++, for the compiler to tell you about it so you can go fix it. (Admittedly it looks like a slippery slope to int square(int n) { return n ^ 2; } — but we can be data-driven here. There are no true-positive instances of ^ 2 in our corpus, whereas there are many true positives of 10 ^ and 2 ^.)

Right, I agree with the "data-driven" bit. Hence my point that if in practice all true positives are from literals, then we should only warn on literals. The examples you posted show that that's not the case.

I now agree that it makes sense to warn when the operands are macros or variables.

[xbolva00]

I now agree that it makes sense to warn when the operands are macros or variables.

I could re-add macro support and then @jfb or @regehr would blame this diagnostic because of macro support =] variables could open a new box of false positives.

Anyway, your motivating case:

minval -= 10 ^ -precision; // https://codesearch.isocpp.org/actcd19/main/q/qgis/qgis_2.18.28+dfsg-2/src/gui/editorwidgets/qgsrangewidgetwrapper.cpp

This should be diagnosted. Location of "-" is not a macro.
Edit: nope, precision is not a macro, just a variable.

real_loop += (((unsigned int) *argv[4]+k) - 48) * 10^(strlen(argv[4]) - (k+1));

Too complex, no chance to diagnose it here :) Not related to macros.

intermediate = (str[offset] - '0') / (10 ^ lpc); // https://codesearch.isocpp.org/actcd19/main/p/pacemaker/pacemaker_1.1.18-2/lib/common/iso8601.c

lpc is not a macro, it is just loop int variable. Not related to macros.

[xbolva00]

I am strongly in favour to just land this as is. :)

[xbolva00]

Another bug found by -Wxor-used-as-pow :)
https://github.com/christinaa/PcapPlusPlus/commit/4646a004f5168bcb78fe2dce78afa08d794c1450

[aaron.ballman]

Adding @rsmith to see if we can make forward progress on this patch again.

[xbolva00]

In D66397#1647455, @aaron.ballman wrote:

Adding @rsmith to see if we can make forward progress on this patch again.

On the other side, I don't want to waste Richard's time since I dont want to extend (variables and macros are controversal topic anyway) it more now. I promised to @jfb to handle (2 ^ 64) - 1 as follow up patch and the promised patch is here..

[aaron.ballman]

In D66397#1649353, @xbolva00 wrote:

In D66397#1647455, @aaron.ballman wrote:

Adding @rsmith to see if we can make forward progress on this patch again.

On the other side, I don't want to waste Richard's time since I dont want to extend (variables and macros are controversal topic anyway) it more now. I promised to @jfb to handle (2 ^ 64) - 1 as follow up patch and the promised patch is here..

It looks like your patch changed the behavior involving macros though, so this isn't just about handling (2 ^ 64) - 1. It's hard to tell due to the way the patch is formatted though. tbh, I would appreciate if you would leave the definition of diagnoseXorMisusedAsPow() where it is and add a forward declare of the function earlier in the file. It would make spotting the differences in the function much easier.

[xbolva00]

Fixed review comment.

[xbolva00]

tbh, I would appreciate if you would leave the definition of diagnoseXorMisusedAsPow() where it is and add a forward declare of the function earlier in the file.

I uploaded the patch almost 2 weeks ago and nobody complained so I thought it is okay. Uploaded, fixed.

[aaron.ballman]

lib/Sema/SemaExpr.cpp

54–55

I'd appreciate some comments on what these arguments should be when non-null.

11145–11146

I would appreciate it if this patch didn't also change the behavior for macros. That seems like a larger discussion that can happen in a separate patch.

11145–11146

Why is this type changing?

11145–11146

Same question here.

11146

!Negative (we already verified above that it's either the + or - operator, so if it's not one, it's the other.)?

11151

The two branches are not equivalent. What about ~0ULL instead?

11154

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.

[aaron.ballman]

In D66397#1652771, @xbolva00 wrote:

tbh, I would appreciate if you would leave the definition of diagnoseXorMisusedAsPow() where it is and add a forward declare of the function earlier in the file.

I uploaded the patch almost 2 weeks ago and nobody complained so I thought it is okay. Uploaded, fixed.

I appreciate it -- this made it much easier to spot the actual changes in the code.

[xbolva00]

Fixed review comments

[xbolva00]

lib/Sema/SemaExpr.cpp

11145–11146

+1, agree.

[aaron.ballman]

lib/Sema/SemaExpr.cpp

11154

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.

[xbolva00]

lib/Sema/SemaExpr.cpp

11154

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, ...

[xbolva00]

lib/Sema/SemaExpr.cpp

11154

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?

[aaron.ballman]

lib/Sema/SemaExpr.cpp

11154

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).

[xbolva00]

lib/Sema/SemaExpr.cpp

11154

@jfb ?

[xbolva00]

lib/Sema/SemaExpr.cpp

11154

(There were no concerns about slowdown in the “base” patch).

Maybe I should just revert it..

[jfb]

lib/Sema/SemaExpr.cpp

11154

I don't really care. My original point was that any suggested fix should be correct, and (1LL << 64) - 1 wasn't.

[aaron.ballman]

lib/Sema/SemaExpr.cpp

11154

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.

[xbolva00]

Ok, I will fix it as you suggested.

lib/Sema/SemaExpr.cpp

 #include "clang/Sema/Template.h"
 #include "llvm/Support/ConvertUTF.h"
 using namespace clang;
 using namespace sema;
 
 /// Determine whether the use of this declaration is valid, without
 /// emitting diagnostics.
 bool Sema::CanUseDecl(NamedDecl *D, bool TreatUnavailableAsInvalid) {
   // See if this is an auto-typed variable whose initializer we are parsing.
   if (ParsingInitForAutoVars.count(D))
     return false;
 
   // See if this is a deleted function.
   if (FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
     if (FD->isDeleted())
       return false;
 
     // If the function has a deduced return type, and we can't deduce it,
static void diagnosePointerIncompatibility(Sema &S, SourceLocation Loc,
                                            Expr *LHSExpr, Expr *RHSExpr) {
   assert(LHSExpr->getType()->isAnyPointerType());
   assert(RHSExpr->getType()->isAnyPointerType());
   S.Diag(Loc, diag::err_typecheck_sub_ptr_compatible)
     << LHSExpr->getType() << RHSExpr->getType() << LHSExpr->getSourceRange()
     << RHSExpr->getSourceRange();
 }
 
+static void diagnoseXorMisusedAsPow(Sema &S, const ExprResult &XorLHS,
+                                    const ExprResult &XorRHS,
+                                    const SourceLocation Loc,
+                                    const Expr *SubLHS = nullptr,
+                                    const Expr *SubRHS = nullptr) {
+  bool Negative = false;
+  const auto *LHSInt = dyn_cast<IntegerLiteral>(XorLHS.get());
+  const auto *RHSInt = dyn_cast<IntegerLiteral>(XorRHS.get());
+
+  if (!LHSInt)
+    return;
+  if (!RHSInt) {
+    // Check negative literals.
+    if (const auto *UO = dyn_cast<UnaryOperator>(XorRHS.get())) {
+      if (UO->getOpcode() != UO_Minus)
+        return;
+      RHSInt = dyn_cast<IntegerLiteral>(UO->getSubExpr());
+      if (!RHSInt)
+        return;
+      Negative = true;
+    } else {
+      return;
+    }
+  }
+
+  if (LHSInt->getValue().getBitWidth() != RHSInt->getValue().getBitWidth())
+    return;
+
+  CharSourceRange ExprRange = CharSourceRange::getCharRange(
+      LHSInt->getBeginLoc(), S.getLocForEndOfToken(RHSInt->getLocation()));
+  std::string ExprStr =
+      Lexer::getSourceText(ExprRange, S.getSourceManager(), S.getLangOpts());
+
+  CharSourceRange XorRange =
+      CharSourceRange::getCharRange(Loc, S.getLocForEndOfToken(Loc));
+  std::string XorStr =
+      Lexer::getSourceText(XorRange, S.getSourceManager(), S.getLangOpts());
+  // Do not diagnose if xor keyword/macro is used.
+  if (XorStr == "xor")
+    return;
+
+  const llvm::APInt &LeftSideValue = LHSInt->getValue();
+  const llvm::APInt &RightSideValue = RHSInt->getValue();
+  const llvm::APInt XorValue = LeftSideValue ^ RightSideValue;
+
+  std::string LHSStr = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(LHSInt->getSourceRange()),
+      S.getSourceManager(), S.getLangOpts());
+  std::string RHSStr = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(RHSInt->getSourceRange()),
+      S.getSourceManager(), S.getLangOpts());
+  if (LHSStr.empty())
+    LHSStr = LeftSideValue.toString(10, false);
+  if (RHSStr.empty())
+    RHSStr = RightSideValue.toString(10, false);
+  if (ExprStr.empty())
+    ExprStr = LHSStr + " ^ " + RHSStr;
+
+  int64_t RightSideIntValue = RightSideValue.getSExtValue();
+  if (Negative) {
+    RightSideIntValue = -RightSideIntValue;
+    RHSStr = "-" + RHSStr;
+  }
+
+  // Do not diagnose 2 ^ 64, but allow special case (2 ^ 64) - 1.
+  if (SubLHS && SubRHS && (LeftSideValue != 2 || RightSideIntValue != 64))
+    return;
+
+  StringRef LHSStrRef = LHSStr;
+  StringRef RHSStrRef = RHSStr;
+  // Do not diagnose binary, hexadecimal, octal literals.
+  if (LHSStrRef.startswith("0b") || LHSStrRef.startswith("0B") ||
+      RHSStrRef.startswith("0b") || RHSStrRef.startswith("0B") ||
+      LHSStrRef.startswith("0x") || LHSStrRef.startswith("0X") ||
+      RHSStrRef.startswith("0x") || RHSStrRef.startswith("0X") ||
+      (LHSStrRef.size() > 1 && LHSStrRef.startswith("0")) ||
+      (RHSStrRef.size() > 1 && RHSStrRef.startswith("0")))
+    return;
+
+  if (LeftSideValue == 2 && RightSideIntValue >= 0) {
+    std::string SuggestedExpr = "1 << " + RHSStr;
+    bool Overflow = false;
+    llvm::APInt One = (LeftSideValue - 1);
+    llvm::APInt PowValue = One.sshl_ov(RightSideValue, Overflow);
+    if (Overflow) {
+      if (RightSideIntValue < 64) {
+        S.Diag(Loc, diag::warn_xor_used_as_pow_base)
+            << ExprStr << XorValue.toString(10, true) << ("1LL << " + RHSStr)
+            << FixItHint::CreateReplacement(ExprRange, "1LL << " + RHSStr);
+      } else if (SubLHS && SubRHS && RightSideIntValue == 64) {
+        const auto *SubInt = dyn_cast<IntegerLiteral>(SubRHS);
+        // Diagnose special case (2 ^ 64) - 1.
+        if (SubInt && SubInt->getValue() == 1) {
+          SuggestedExpr = S.getPreprocessor().isMacroDefined("ULLONG_MAX")
+                              ? "ULLONG_MAX"
+                              : "-1LL";
+          ExprRange = CharSourceRange::getCharRange(
+              SubLHS->getBeginLoc(),
+              S.getLocForEndOfToken(SubInt->getLocation()));
+          S.Diag(Loc, diag::warn_xor_used_as_pow_base)
+              << ExprStr << XorValue.toString(10, true) << SuggestedExpr
+              << PowValue.toString(10, true)
+              << FixItHint::CreateReplacement(ExprRange, SuggestedExpr);
+        } else {
+          return;
+        }
+      } else {
+        return;
+      }
+    } else {
+      S.Diag(Loc, diag::warn_xor_used_as_pow_base_extra)
+          << ExprStr << XorValue.toString(10, true) << SuggestedExpr
+          << PowValue.toString(10, true)
+          << FixItHint::CreateReplacement(
+                 ExprRange, (RightSideIntValue == 0) ? "1" : SuggestedExpr);
+    }
+
+    S.Diag(Loc, diag::note_xor_used_as_pow_silence) << ("0x2 ^ " + RHSStr);
+  } else if (LeftSideValue == 10) {
+    std::string SuggestedValue = "1e" + std::to_string(RightSideIntValue);
+    S.Diag(Loc, diag::warn_xor_used_as_pow_base)
+        << ExprStr << XorValue.toString(10, true) << SuggestedValue
+        << FixItHint::CreateReplacement(ExprRange, SuggestedValue);
+    S.Diag(Loc, diag::note_xor_used_as_pow_silence) << ("0xA ^ " + RHSStr);
+  }
+}
+
 // C99 6.5.6
 QualType Sema::CheckAdditionOperands(ExprResult &LHS, ExprResult &RHS,
                                      SourceLocation Loc, BinaryOperatorKind Opc,
                                      QualType* CompLHSTy) {
   checkArithmeticNull(*this, LHS, RHS, Loc, /*IsCompare=*/false);
 
   if (LHS.get()->getType()->isVectorType() ||
       RHS.get()->getType()->isVectorType()) {
if (LHS.get()->getType()->isVectorType() ||
     if (CompLHSTy) *CompLHSTy = compType;
     return compType;
   }
 
   QualType compType = UsualArithmeticConversions(LHS, RHS, CompLHSTy);
   if (LHS.isInvalid() || RHS.isInvalid())
     return QualType();
 
+  const auto *BO = dyn_cast<BinaryOperator>(LHS.get()->IgnoreParens());
+  if (BO && BO->getOpcode() == BO_Xor)
+    diagnoseXorMisusedAsPow(*this, BO->getLHS(), BO->getRHS(), Loc, LHS.get(),
+                            RHS.get());
+
   // Enforce type constraints: C99 6.5.6p3.
 
   // Handle the common case first (both operands are arithmetic).
   if (!compType.isNull() && compType->isArithmeticType()) {
     if (CompLHSTy) *CompLHSTy = compType;
     return compType;
   }
 
if (BinaryOperator::isEqualityOp(Opc) &&
       LHSType->hasFloatingRepresentation()) {
     assert(RHS.get()->getType()->hasFloatingRepresentation());
     CheckFloatComparison(Loc, LHS.get(), RHS.get());
   }
 
   // Return a signed type for the vector.
   return GetSignedVectorType(vType);
 }
 
-static void diagnoseXorMisusedAsPow(Sema &S, ExprResult &LHS, ExprResult &RHS,
-                                    SourceLocation Loc) {
-  // Do not diagnose macros.
-  if (Loc.isMacroID())
-    return;
-
-  bool Negative = false;
-  const auto *LHSInt = dyn_cast<IntegerLiteral>(LHS.get());
-  const auto *RHSInt = dyn_cast<IntegerLiteral>(RHS.get());
-
-  if (!LHSInt)
-    return;
-  if (!RHSInt) {
-    // Check negative literals.
-    if (const auto *UO = dyn_cast<UnaryOperator>(RHS.get())) {
-      if (UO->getOpcode() != UO_Minus)
-        return;
-      RHSInt = dyn_cast<IntegerLiteral>(UO->getSubExpr());
-      if (!RHSInt)
-        return;
-      Negative = true;
-    } else {
-      return;
-    }
-  }
-
-  if (LHSInt->getValue().getBitWidth() != RHSInt->getValue().getBitWidth())
-    return;
-
-  CharSourceRange ExprRange = CharSourceRange::getCharRange(
-      LHSInt->getBeginLoc(), S.getLocForEndOfToken(RHSInt->getLocation()));
-  llvm::StringRef ExprStr =
-      Lexer::getSourceText(ExprRange, S.getSourceManager(), S.getLangOpts());
-
-  CharSourceRange XorRange =
-      CharSourceRange::getCharRange(Loc, S.getLocForEndOfToken(Loc));
-  llvm::StringRef XorStr =
-      Lexer::getSourceText(XorRange, S.getSourceManager(), S.getLangOpts());
-  // Do not diagnose if xor keyword/macro is used.
-  if (XorStr == "xor")
-    return;
-
-  const llvm::APInt &LeftSideValue = LHSInt->getValue();
-  const llvm::APInt &RightSideValue = RHSInt->getValue();
-  const llvm::APInt XorValue = LeftSideValue ^ RightSideValue;
-
-  std::string LHSStr = Lexer::getSourceText(
-      CharSourceRange::getTokenRange(LHSInt->getSourceRange()),
-      S.getSourceManager(), S.getLangOpts());
-  std::string RHSStr = Lexer::getSourceText(
-      CharSourceRange::getTokenRange(RHSInt->getSourceRange()),
-      S.getSourceManager(), S.getLangOpts());
-
-  int64_t RightSideIntValue = RightSideValue.getSExtValue();
-  if (Negative) {
-    RightSideIntValue = -RightSideIntValue;
-    RHSStr = "-" + RHSStr;
-  }
-
-  StringRef LHSStrRef = LHSStr;
-  StringRef RHSStrRef = RHSStr;
-  // Do not diagnose binary, hexadecimal, octal literals.
-  if (LHSStrRef.startswith("0b") || LHSStrRef.startswith("0B") ||
-      RHSStrRef.startswith("0b") || RHSStrRef.startswith("0B") ||
-      LHSStrRef.startswith("0x") || LHSStrRef.startswith("0X") ||
-      RHSStrRef.startswith("0x") || RHSStrRef.startswith("0X") ||
-      (LHSStrRef.size() > 1 && LHSStrRef.startswith("0")) ||
-      (RHSStrRef.size() > 1 && RHSStrRef.startswith("0")))
-    return;
-
-  if (LeftSideValue == 2 && RightSideIntValue >= 0) {
-    std::string SuggestedExpr = "1 << " + RHSStr;
-    bool Overflow = false;
-    llvm::APInt One = (LeftSideValue - 1);
-    llvm::APInt PowValue = One.sshl_ov(RightSideValue, Overflow);
-    if (Overflow) {
-      if (RightSideIntValue < 64)
-        S.Diag(Loc, diag::warn_xor_used_as_pow_base)
-            << ExprStr << XorValue.toString(10, true) << ("1LL << " + RHSStr)
-            << FixItHint::CreateReplacement(ExprRange, "1LL << " + RHSStr);
-      else
-         // TODO: 2 ^ 64 - 1
-        return;
-    } else {
-      S.Diag(Loc, diag::warn_xor_used_as_pow_base_extra)
-          << ExprStr << XorValue.toString(10, true) << SuggestedExpr
-          << PowValue.toString(10, true)
-          << FixItHint::CreateReplacement(
-                 ExprRange, (RightSideIntValue == 0) ? "1" : SuggestedExpr);
-    }
-
-    S.Diag(Loc, diag::note_xor_used_as_pow_silence) << ("0x2 ^ " + RHSStr);
-  } else if (LeftSideValue == 10) {
-    std::string SuggestedValue = "1e" + std::to_string(RightSideIntValue);
-    S.Diag(Loc, diag::warn_xor_used_as_pow_base)
-        << ExprStr << XorValue.toString(10, true) << SuggestedValue
-        << FixItHint::CreateReplacement(ExprRange, SuggestedValue);
-    S.Diag(Loc, diag::note_xor_used_as_pow_silence) << ("0xA ^ " + RHSStr);
-  }
-}
-
 QualType Sema::CheckVectorLogicalOperands(ExprResult &LHS, ExprResult &RHS,
                                           SourceLocation Loc) {
   // Ensure that either both operands are of the same vector type, or
   // one operand is of a vector type and the other is of its element type.
   QualType vType = CheckVectorOperands(LHS, RHS, Loc, false,
                                        /*AllowBothBool*/true,
                                        /*AllowBoolConversions*/false);
   if (vType.isNull())
     return InvalidOperands(Loc, LHS, RHS);
   if (getLangOpts().OpenCL && getLangOpts().OpenCLVersion < 120 &&
       !getLangOpts().OpenCLCPlusPlus && vType->hasFloatingRepresentation())
     return InvalidOperands(Loc, LHS, RHS);
   // FIXME: The check for C++ here is for GCC compatibility. GCC rejects the
   //        usage of the logical operators && and || with vectors in C. This
   //        check could be notionally dropped.
   if (!getLangOpts().CPlusPlus &&
       !(isa<ExtVectorType>(vType->getAs<VectorType>())))
ExprResult Sema::ActOnObjCAvailabilityCheckExpr(
   if (getCurFunctionOrMethodDecl())
     getEnclosingFunction()->HasPotentialAvailabilityViolations = true;
   else if (getCurBlock() || getCurLambda())
     getCurFunction()->HasPotentialAvailabilityViolations = true;
 
   return new (Context)
       ObjCAvailabilityCheckExpr(Version, AtLoc, RParen, Context.BoolTy);
 }
 No newline at end of file

test/SemaCXX/warn-xor-as-pow.cpp

 // RUN: %clang_cc1 -x c -fsyntax-only -verify -Wxor-used-as-pow %s
 // RUN: %clang_cc1 -x c -fsyntax-only -verify %s
 // RUN: %clang_cc1 -x c -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 // RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wxor-used-as-pow %s
 // RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s
 // RUN: %clang_cc1 -x c++ -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 #define FOOBAR(x, y) (x * y)
 #define XOR(x, y) (x ^ y)
 #define TWO 2
 #define TEN 10
+#define IOP 64
 #define TWO_ULL 2ULL
 #define EPSILON 10 ^ -300
+#define ALPHA_OFFSET 3
 
 #define flexor 7
 
 #ifdef __cplusplus
 constexpr long long operator"" _xor(unsigned long long v) { return v; }
 
 constexpr long long operator"" _0b(unsigned long long v) { return v; }
 constexpr long long operator"" _0X(unsigned long long v) { return v; }
 #else
-#define xor    ^ // iso646.h
+#define xor ^ // iso646.h
 #endif
 
 void test(unsigned a, unsigned b) {
   unsigned res;
   res = a ^ 5;
   res = 2 ^ b;
   res = a ^ b;
   res = 2 ^ -1;
void test(unsigned a, unsigned b) {
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:16}:"1 << 8"
   // expected-note@-2 {{replace expression with '0x2 ^ 8' to silence this warning}}
   res = 2 ^ 16; // expected-warning {{result of '2 ^ 16' is 18; did you mean '1 << 16' (65536)?}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:15}:"1 << 16"
   // expected-note@-2 {{replace expression with '0x2 ^ 16' to silence this warning}}
   res = 2 ^ TEN; // expected-warning {{result of '2 ^ TEN' is 8; did you mean '1 << TEN' (1024)?}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:16}:"1 << TEN"
   // expected-note@-2 {{replace expression with '0x2 ^ TEN' to silence this warning}}
+  res = res + (2 ^ ALPHA_OFFSET); // expected-warning {{result of '2 ^ ALPHA_OFFSET' is 1; did you mean '1 << ALPHA_OFFSET' (8)?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:16-[[@LINE-1]]:32}:"1 << ALPHA_OFFSET"
+  // expected-note@-2 {{replace expression with '0x2 ^ ALPHA_OFFSET' to silence this warning}}
   res = 0x2 ^ 16;
   res = 2 xor 16;
 
   res = 2 ^ 0x4;
   res = 2 ^ 04;
   res = 0x2 ^ 10;
   res = 0X2 ^ 10;
   res = 02 ^ 10;
   res = FOOBAR(2, 16);
   res = 0b10 ^ 16;
   res = 0B10 ^ 16;
   res = 2 ^ 0b100;
-  res = XOR(2, 16);
+  res = XOR(2, 16); // expected-warning {{result of '2 ^ 16' is 18; did you mean '1 << 16' (65536)?}}
+  // expected-note@-1 {{replace expression with '0x2 ^ 16' to silence this warning}}
   unsigned char two = 2;
   res = two ^ 16;
   res = TWO_ULL ^ 16;
   res = 2 ^ 32; // expected-warning {{result of '2 ^ 32' is 34; did you mean '1LL << 32'?}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:15}:"1LL << 32"
   // expected-note@-2 {{replace expression with '0x2 ^ 32' to silence this warning}}
-  res = 2 ^ 64;
+  res = (2 ^ 64) - 1; // expected-warning {{result of '2 ^ 64' is 66; did you mean '-1LL'?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:21}:"-1LL"
+  // expected-note@-2 {{replace expression with '0x2 ^ 64' to silence this warning}}
+#define ULLONG_MAX 18446744073709551615ULL
+  res = (2 ^ 64) - 1; // expected-warning {{result of '2 ^ 64' is 66; did you mean 'ULLONG_MAX'?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:21}:"ULLONG_MAX"
+  // expected-note@-2 {{replace expression with '0x2 ^ 64' to silence this warning}}
+  res = (2 ^ 64) - 2;
+  res = (2 ^ IOP) - 1; // expected-warning {{result of '2 ^ IOP' is 66; did you mean 'ULLONG_MAX'?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:22}:"ULLONG_MAX"
+  // expected-note@-2 {{replace expression with '0x2 ^ IOP' to silence this warning}}
+  res = (2 ^ 65) - 1;
+  res = (2 + 64) - 1;
+  res = (3 ^ 64) - 1;
+  res = (2 ^ 8) - 1; // expected-warning {{result of '2 ^ 8' is 10; did you mean '1 << 8' (256)?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:15}:"1 << 8"
+  // expected-note@-2 {{replace expression with '0x2 ^ 8' to silence this warning}}
 
-  res = EPSILON;
+  res = EPSILON; // expected-warning {{result of 'EPSILON' is 294; did you mean '1e-300'?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:16}:"1e-300"
+  // expected-note@-2 {{replace expression with '0xA ^ -300' to silence this warning}}
   res = 10 ^ 0; // expected-warning {{result of '10 ^ 0' is 10; did you mean '1e0'?}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:15}:"1e0"
   // expected-note@-2 {{replace expression with '0xA ^ 0' to silence this warning}}
   res = 10 ^ 1; // expected-warning {{result of '10 ^ 1' is 11; did you mean '1e1'?}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:15}:"1e1"
   // expected-note@-2 {{replace expression with '0xA ^ 1' to silence this warning}}
   res = 10 ^ 2; // expected-warning {{result of '10 ^ 2' is 8; did you mean '1e2'?}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:15}:"1e2"
   // expected-note@-2 {{replace expression with '0xA ^ 2' to silence this warning}}
   res = 10 ^ 4; // expected-warning {{result of '10 ^ 4' is 14; did you mean '1e4'?}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:15}:"1e4"
   // expected-note@-2 {{replace expression with '0xA ^ 4' to silence this warning}}
   res = 10 ^ 10; // expected-warning {{result of '10 ^ 10' is 0; did you mean '1e10'?}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:16}:"1e10"
   // expected-note@-2 {{replace expression with '0xA ^ 10' to silence this warning}}
   res = TEN ^ 10; // expected-warning {{result of 'TEN ^ 10' is 0; did you mean '1e10'?}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:17}:"1e10"
   // expected-note@-2 {{replace expression with '0xA ^ 10' to silence this warning}}
+  res = 10 ^ TEN; // expected-warning {{result of '10 ^ TEN' is 0; did you mean '1e10'?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:17}:"1e10"
+  // expected-note@-2 {{replace expression with '0xA ^ TEN' to silence this warning}}
   res = 10 ^ 100; // expected-warning {{result of '10 ^ 100' is 110; did you mean '1e100'?}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:17}:"1e100"
   // expected-note@-2 {{replace expression with '0xA ^ 100' to silence this warning}}
   res = 0xA ^ 10;
   res = 10 xor 10;
 #ifdef __cplusplus
   res = 10 ^ 5_xor;
   res = 10_xor ^ 5;
   res = 10 ^ 5_0b;
   res = 10_0X ^ 5;
 #endif
 }