[Diagnostics] Diagnose misused xor as pow

[xbolva00]

Summary

Motivation:
https://twitter.com/jfbastien/status/1139298419988549632
https://twitter.com/mikemx7f/status/1139335901790625793
https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=10+%5E&search=Search

[xbolva00]

include/clang/Basic/DiagnosticGroups.td

508 ↗

(On Diff #205049)

GCC folks will do this diagnostic soon too, so I will wait and use their name.

[xbolva00]

More tests

[xbolva00]

Should we also emit a note how to silence it, eg. swap xor operands or add parentheses around 2/10 ?

[xbolva00]

Silence note added.

[jfb]

As a reference: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90885

[jfb]

I think you want to avoid warning inside macros as well, say:

#define AWESOME(x, y) ({ blah blah blah; x ^ y; blah })

AWESOME(2, 10); // probably not a bug

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

13 ↗

(On Diff #205089)

2 ^ 0 seems like it would be a bug too?

[jfb]

What does the suggested for for 2 ^ 32 look like? I hope it's not 1 << 32 :)

[xbolva00]

Fixed notes by @jfb. Thank you.

[jfb]

Why have .c and .cpp tests?

test/Sema/warn-xor-as-pow.c

43 ↗

(On Diff #205121)

I think we want to suggest 1LL << 32 or something like that. Whatever we usually do with this type of suggestion.

45 ↗

(On Diff #205121)

This one hits a ceiling, we can't really suggest anything for this value IMO. Maybe we need to see if the user is doing 2^64 - 1? In that case we can offer a suggestion.

[xbolva00]

test/Sema/warn-xor-as-pow.c

45 ↗

(On Diff #205121)

Since this requires changes in other parts of code to catch 2^64 - 1, maybe we can leave it as a follow-up patch?

[jfb]

test/Sema/warn-xor-as-pow.c

45 ↗

(On Diff #205121)

Sure.

[• Quuxplusone]

Seems low-value at first glance, but I guess I can't argue with those Twitter and codesearch results.

Do you have codesearch results for ^2? https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=%5E+2&search=Search Seems like a lot of false positives (and a lot of code comments turning up in codesearch??), but would be perhaps even more valuable to the kinds of users who would write 10^x or x^2.

What is going on in this case, and would a warning here be a false positive or a true positive? https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=10+%5E+7&search=Search

lib/Sema/SemaExpr.cpp

10913 ↗

(On Diff #205121)

Why do you special-case 0b prefix (or suffix — 0x0b is also special-cased by this snippet), but you don't special-case octal or hexadecimal constants? I'm sure x ^ 0x1234 will be a more common spelling than x ^ 0b1001000110100.

10924 ↗

(On Diff #205121)

Do you have metrics indicating that this line is an improvement over if (true) {?

10959 ↗

(On Diff #205121)

I don't understand why parenthesizing one argument should silence the warning. Wouldn't it be more natural to suggest converting both arguments to hexadecimal or binary? That is, convert 10 ^ x to 0xA ^ x, or 2 ^ 16 to 0x2 ^ 0x10.

test/Sema/warn-xor-as-pow.c

38 ↗

(On Diff #205121)

Please add test cases for 2 ^ 0x4 and 2 ^ 04 and 0x2 ^ 10 and 02 ^ 10.

39 ↗

(On Diff #205121)

I don't understand why this line doesn't warn. Is it because the macro's name has the case-insensitive string xor in it? Is it because there is a macro involved at all? In either case, please add another test case for res = FOOBAR(2, 16).

Also res = EPSILON where #define EPSILON 10^-300. That seems to come up in the codesearch results a lot.

[xbolva00]

lib/Sema/SemaExpr.cpp

10924 ↗

(On Diff #205121)

This is left over of older code :) I will fix it.

test/Sema/warn-xor-as-pow.c

39 ↗

(On Diff #205121)

EPSILON 10^-300

this is in macro :(

so maybe if negative RHS, we should check macro too? @jfb

[xbolva00]

Updated.

[xbolva00]

Removed useless else block with return.

[xbolva00]

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

85 ↗

(On Diff #205140)

What should we suggest here ? :) Nevative shift ? think we should diagnose it.

I am not sure about 10 ^ 0 case... warn or not ?

@jfb

[regehr]

Just wanted to say that I think I agree with the design choices here!

[xbolva00]

Handle 10 ^ -C case..

[• Quuxplusone]

lib/Sema/SemaExpr.cpp

10931 ↗

(On Diff #205187)

Can you use starts_with (or the LLVM equivalent) in both of these cases? It'll be faster and also more correct.

Hex and binary are handled up here on line 10927, but octal is handled down on line 10955; why? Can't they be combined into one place in the code?

10982 ↗

(On Diff #205187)

Suppressing the warning specifically on 10 ^ 0 (which means 10) seems like an anti-feature.

10988 ↗

(On Diff #205187)

I suggest at least one unit test case that involves 10 ^ 100, and considering the user-friendliness of the resulting error message. How about suggesting "1e" + std::to_string(RightSideIntValue) as the fixit in both cases?

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

52 ↗

(On Diff #205187)

I still expect to see a warning either on this line, or on the line where the macro is defined. I don't see why we'd want to be silent in this case.

[xbolva00]

Thanks, I will address your notes soon!

The only remaining question is, as your said, whether or not to diagnose xors in macros. @regerh @jfb ?

[xbolva00]

lib/Sema/SemaExpr.cpp

10931 ↗

(On Diff #205187)

We cannot use starts_with here, case: 2 ^ 0b11.

Yes, I can combine it to one place.

[regehr]

In D63423#1547216, @xbolva00 wrote:

The only remaining question is, as your said, whether or not to diagnose xors in macros. @regerh @jfb ?

IMO it's better to not warn about xors in macros-- I like the current tradeoffs.

[xbolva00]

Addressed review notes.

[xbolva00]

In D63423#1547244, @regehr wrote:

In D63423#1547216, @xbolva00 wrote:

The only remaining question is, as your said, whether or not to diagnose xors in macros. @regerh @jfb ?

IMO it's better to not warn about xors in macros-- I like the current tradeoffs.

Thank you for opinion.

I think this patch is (almost) done. :)

[xbolva00]

Removed unused diagnostic message.

[xbolva00]

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

13 ↗

(On Diff #205089)

Small question..

Instead of 1<<0 for this case, I emit fixit "1", seems ok for you?

[aaron.ballman]

include/clang/Basic/DiagnosticSemaKinds.td

3306 ↗

(On Diff #205202)

We usually use "did you mean" in these kinds of diagnostics. Replace the comma with a semicolon.

Same comments below.

lib/Sema/SemaExpr.cpp

10895 ↗

(On Diff #205202)

Missing a full stop at the end of the comment. Same comment applies elsewhere.

[aaron.ballman]

In D63423#1547244, @regehr wrote:

In D63423#1547216, @xbolva00 wrote:

The only remaining question is, as your said, whether or not to diagnose xors in macros. @regerh @jfb ?

IMO it's better to not warn about xors in macros-- I like the current tradeoffs.

Do you mind explaining why? I would have expected to diagnose an xor within a macro because the code is just as wrong there, but maybe I'm missing something.

[xbolva00]

We will be noisy in this case

#define IRQ_CHINT4_LEVEL        (11 ^ 7)
#define IRQ_CHINT3_LEVEL        (10 ^ 7)
#define IRQ_CHINT2_LEVEL        (9 ^ 7)
#define IRQ_CHINT1_LEVEL        (8 ^ 7)

[aaron.ballman]

In D63423#1550563, @xbolva00 wrote:

We will be noisy in this case

#define IRQ_CHINT4_LEVEL        (11 ^ 7)
#define IRQ_CHINT3_LEVEL        (10 ^ 7)
#define IRQ_CHINT2_LEVEL        (9 ^ 7)
#define IRQ_CHINT1_LEVEL        (8 ^ 7)

Sure, but do we not want to be noisy in a case like: #define BAD_IDEA (2 ^ 16)?

[xbolva00]

... But I go thru codesearch and this is just one case I think.

I think we should warn in macros... In codesearch case ,they are many more true positives, then false negatives.

#define MAX_T_U32 ((2^32)-1)

Ouch.

[aaron.ballman]

In D63423#1550586, @xbolva00 wrote:

... But I go thru codesearch and this is just one case I think.

I think we should warn in macros... In codesearch case ,they are many more true positives, then false negatives.

#define MAX_T_U32 ((2^32)-1)

Ouch.

That's my rationale as well. If it happens outside of macros and is bad, it's probably just as likely to happen within a macro and be equally as bad. I think we probably should warn within macros.

[xbolva00]

Added support to warn in macros.
Renamed to -Wxor-used-as-pow.

[jfb]

What I meant with macros was that I don't think we should warn on:

#define LEGIT(a, b) ({ work work work; a ^ b; work work work; })

LEGIT(10, 5);

If the constants are inline in the macros then sure, warn there. Basically: if you literally wrote CONSTANT ^ CONSTANT and it looks fishy, let's warn. If token-pasting wrote it for you and it looks fishy, let's not warn.

[aaron.ballman]

In D63423#1550697, @jfb wrote:

What I meant with macros was that I don't think we should warn on:

#define LEGIT(a, b) ({ work work work; a ^ b; work work work; })

LEGIT(10, 5);

If the constants are inline in the macros then sure, warn there. Basically: if you literally wrote CONSTANT ^ CONSTANT and it looks fishy, let's warn. If token-pasting wrote it for you and it looks fishy, let's not warn.

So you would not want to see this code diagnosed?

#define POW(x, y) (x ^ y)

void f() {
  int two_to_the_derp = POW(2, 16);
}

I'm not certain I understand the rationale for why we would not want to diagnose such a construct. Do we have reason to expect a lot of false positives?

[xbolva00]

In D63423#1550697, @jfb wrote:

What I meant with macros was that I don't think we should warn on:

#define LEGIT(a, b) ({ work work work; a ^ b; work work work; })

LEGIT(10, 5);

If the constants are inline in the macros then sure, warn there. Basically: if you literally wrote CONSTANT ^ CONSTANT and it looks fishy, let's warn. If token-pasting wrote it for you and it looks fishy, let's not warn.

Is it worth to pattern match this case? I don't like how this diagnostic is getting more and more complicated.

User can use -Wno-xor-used-as-pow if auto-generated weird code.

Or I can move it under -Wextra..

[jfb]

In D63423#1550705, @aaron.ballman wrote:

In D63423#1550697, @jfb wrote:

What I meant with macros was that I don't think we should warn on:

#define LEGIT(a, b) ({ work work work; a ^ b; work work work; })

LEGIT(10, 5);

If the constants are inline in the macros then sure, warn there. Basically: if you literally wrote CONSTANT ^ CONSTANT and it looks fishy, let's warn. If token-pasting wrote it for you and it looks fishy, let's not warn.

So you would not want to see this code diagnosed?

#define POW(x, y) (x ^ y)

void f() {
  int two_to_the_derp = POW(2, 16);
}

I'm not certain I understand the rationale for why we would not want to diagnose such a construct. Do we have reason to expect a lot of false positives?

If you wrote CONSTANT_2_OR_10 ^ CONSTANT I'm pretty sure you messed up. Macros are opaque, and I'm not sure you clearly messed up anymore. Let's take the clear win, see what falls out, and only expand our reach if we have reason to do so. Without trying on huge codebases I don't know whether we'll get a lot of false positive. I'd rather be conservative and not cause headache.

[regehr]

I am also in the "be a bit conservative at first and see how things shake out" camp, but it's y'all doing the hard work here, not me :)

[xbolva00]

Reverted macro support.

[xbolva00]

Okey, it should be fine now.

If you are agree with the current state, please approve it.

[lebedev.ri]

I've always been frustrated at how clang just gives up as soon as it sees a macro.
At best that should be controlled with some command-line flag.

[jfb]

In D63423#1550725, @lebedev.ri wrote:

I've always been frustrated at how clang just gives up as soon as it sees a macro.
At best that should be controlled with some command-line flag.

This patch is not the place to change common clang behavior. I'm advocating for what we usually do: be conservative in macros because token-pasting tends to look "wrong" but not actually be wrong as much (i.e. false positives). If you want clang to do something different, an RFC to cfe-dev is the right place for such a discussion.

[xbolva00]

In D63423#1550725, @lebedev.ri wrote:

I've always been frustrated at how clang just gives up as soon as it sees a macro.
At best that should be controlled with some command-line flag.

I cannot promise anything, but maybe I could do -Wxor-used-as-pow-in-macro (off by default) so people could try it and see how noisy it would be :)

Edit: Oh, probably no, I would have to duplicate items in DiagnosticSemaKinds :(

[lebedev.ri]

In D63423#1550728, @jfb wrote:

In D63423#1550725, @lebedev.ri wrote:

I've always been frustrated at how clang just gives up as soon as it sees a macro.
At best that should be controlled with some command-line flag.

This patch is not the place to change common clang behavior. I'm advocating for what we usually do: be conservative in macros because token-pasting tends to look "wrong" but not actually be wrong as much (i.e. false positives). If you want clang to do something different, an RFC to cfe-dev is the right place for such a discussion.

I don't think i'm proposing to change the entirety of warnings to warn on macros, am i?
Likewise, is it a causality/practice to ignore macros, or a documented requirement [that would require RFC for changing]?
It's been awhile since i've read through the entirety of docs, but i don't recall seeing one.

In D63423#1550732, @xbolva00 wrote:

In D63423#1550725, @lebedev.ri wrote:

I've always been frustrated at how clang just gives up as soon as it sees a macro.
At best that should be controlled with some command-line flag.

I cannot promise nothing, but maybe I could do -Wxor-used-as-pow-in-macro (off by default) so people could try it and see how noisy it would be :)

I'm not sure -Wflag + -Wflag-in-macros will scale :/
i was thinking more along the lines of warn-for-system-headers flag.

[aaron.ballman]

In D63423#1550713, @jfb wrote:

In D63423#1550705, @aaron.ballman wrote:

In D63423#1550697, @jfb wrote:

What I meant with macros was that I don't think we should warn on:

#define LEGIT(a, b) ({ work work work; a ^ b; work work work; })

LEGIT(10, 5);

If the constants are inline in the macros then sure, warn there. Basically: if you literally wrote CONSTANT ^ CONSTANT and it looks fishy, let's warn. If token-pasting wrote it for you and it looks fishy, let's not warn.

So you would not want to see this code diagnosed?

#define POW(x, y) (x ^ y)

void f() {
  int two_to_the_derp = POW(2, 16);
}

I'm not certain I understand the rationale for why we would not want to diagnose such a construct. Do we have reason to expect a lot of false positives?

If you wrote CONSTANT_2_OR_10 ^ CONSTANT I'm pretty sure you messed up. Macros are opaque, and I'm not sure you clearly messed up anymore. Let's take the clear win, see what falls out, and only expand our reach if we have reason to do so. Without trying on huge codebases I don't know whether we'll get a lot of false positive. I'd rather be conservative and not cause headache.

IIRC, we do not usually go out of our way to stop diagnosing code within macro expansions unless there's a reason to do so. Given that we know the code is likely wrong (because we're only checking constants whether inside or outside of a macro expansion) and that we've seen the incorrect code happen inside of macros in the wild, I suspect that being conservative is not much of a win. That said, I definitely understand the desire to not issue burdensome amounts of false positives. If we see false positives coming from macros, we should address them (perhaps even by not diagnosing within a macro). Perhaps the author can run the check over a large corpus of code to see whether fps come up in practice? (The presence of fps will suggest to not warn in macros, but the absence of fps won't tell us too much.)

[xbolva00]

Added dots in the comments.

[xbolva00]

Perhaps the author can run the check over a large corpus of code to see whether fps come up in practice? (The presence of fps will suggest to not warn in macros, but the absence of fps won't tell us too much.)

Sorry, I have no time nor spare computers to check big codebases. As @jfb said, let's start with a conservative approach and possibly improve it in the future.

[aaron.ballman]

In D63423#1550768, @xbolva00 wrote:

Perhaps the author can run the check over a large corpus of code to see whether fps come up in practice? (The presence of fps will suggest to not warn in macros, but the absence of fps won't tell us too much.)

Sorry, I have no time nor spare computers to check big codebases. As @jfb said, let's start with a conservative approach and possibly improve it in the future.

I can live with that, but my concern is that we often don't do that "improve it in the future" bit and we already know about buggy cases in the wild where the diagnostic will not trigger. Given how often people use macros for simple, but wrong, constructs like my POW example, I think this reduces the utility of the check.

What are the GCC folks planning to do with macros?

[xbolva00]

David Malcon - GCC: “I think we'd want to *not* warn if either of the operands are from a macro expansion.“

But I see no reason to follow this and why we should restrict this even more..

[xbolva00]

Improved code, added const.

[xbolva00]

Any comments? If no, please approve..

[jfb]

include/clang/Basic/DiagnosticGroups.td

508 ↗

(On Diff #205692)

Does this match the naming that GCC ended up with?

include/clang/Basic/DiagnosticSemaKinds.td

3309 ↗

(On Diff #205692)

This is still a bad diagnostic, I'd rather see it fixed.

lib/Sema/SemaExpr.cpp

10929 ↗

(On Diff #205692)

Doesn't this match any expression that contains xor? Put another way, I don't see "xor" used anywhere else under clang, what's usually done?

10951 ↗

(On Diff #205692)

This ignores 0B.

10954 ↗

(On Diff #205692)

This ignores 0X.

10961 ↗

(On Diff #205692)

The above seems to mishandle user-defined literals. Unless the UDL contains 0x or 0b in them. I'd like to see this tested.

[• Quuxplusone]

lib/Sema/SemaExpr.cpp

10950 ↗

(On Diff #205692)

I would rather see this whole section as a three-liner:

// Do not diagnose binary, octal, or hexadecimal literals.
if (StringRef(LHSStr).startswith("0") || StringRef(RHSStr).startswith("0"))
    return;

10963 ↗

(On Diff #205692)

Here and on line 10971, I suggest whitespace around the << (in the suggested expression).

10983 ↗

(On Diff #205692)

Do you currently warn on, let's say, 2uLL ^ 7? and if so, do we care that the suggested expression 0x2 ^ 7 has a different type? I imagine the answer to "do we care" is "no," but I thought I should ask.

[xbolva00]

include/clang/Basic/DiagnosticGroups.td

508 ↗

(On Diff #205692)

No, they have no progress yet. GCC should not block our progress here..

include/clang/Basic/DiagnosticSemaKinds.td

3309 ↗

(On Diff #205692)

Why so? Can you propose wording here?

We provide a fixit for it, e.g. 1LL<<32..

lib/Sema/SemaExpr.cpp

10929 ↗

(On Diff #205692)

Yes, but since xor is keyword in C++, I think this is safe.

[xbolva00]

lib/Sema/SemaExpr.cpp

10983 ↗

(On Diff #205692)

We check if bidwith is same.. Bail out if not.

[• Quuxplusone]

lib/Sema/SemaExpr.cpp

10929 ↗

(On Diff #205692)

I believe JF is worried about expressions like constexpr int flexor_exponent = 3; return 10 ^ flexor_exponent;. That expression contains the substring "xor" but does not use the xor keyword. Which reminds me, can you add some test cases showing what behavior you expect for operands that are not integer literals but still compile-time const, or constexpr, or template arguments?

[xbolva00]

lib/Sema/SemaExpr.cpp

10929 ↗

(On Diff #205692)

Ah, I will change it to “ xor “.

[jfb]

lib/Sema/SemaExpr.cpp

10929 ↗

(On Diff #205692)

Spaces aren't the only valid whitespace character :)

2 xor
   31

[xbolva00]

lib/Sema/SemaExpr.cpp

10929 ↗

(On Diff #205692)

I will rework it to chech only XOR op, not the while expr.

[xbolva00]

lib/Sema/SemaExpr.cpp

10929 ↗

(On Diff #205692)

Well, 10 ^ flexor ..

We cannot reach xor check, flexor is not a integer literal. So I will rework nothing here.

[aaron.ballman]

lib/Sema/SemaExpr.cpp

10929 ↗

(On Diff #205692)

#define flexor 7

Now flexor is an integer literal. (A test case for this would be beneficial.)

[jfb]

lib/Sema/SemaExpr.cpp

10929 ↗

(On Diff #205692)

Also, as I've said above:

constexpr long long operator"" _xor(unsigned long long v)
{ return v; }
auto i = 10 ^ 5_xor;

[xbolva00]

Addressed review notes.

[xbolva00]

Hello,

I fixed your notes + added test cases.

Only remaining question is what to do in "2 ^ 64" (65, 66, .. etc) case. I can emit "result of 2 ^ 64" is "XYZ", but this is rather poor diagnostic. What should we suggest for "2 ^ 64"? (@jfb

The "2 ^ 64 - 1" case to be solved as follow up patch..

And since there is no progress on GCC side, I will use -Wxor-used-as-pow

[xbolva00]

svn added forgotten test file..

[xbolva00]

Added UDL tests

[aaron.ballman]

include/clang/Basic/DiagnosticGroups.td

508 ↗

(On Diff #205692)

Agreed, but we should still coordinate to ensure the names we pick are the same since this is a user-facing option. If GCC isn't adding this yet, can you file a bug report (or comment on an existing one) with the name we've chosen?

lib/Sema/SemaExpr.cpp

10977–10980 ↗

(On Diff #214533)

if (const auto *UO = dyn_cast<UnaryOperator>(RHS.get())) { ... } rather than doing an isa<> followed by a cast<>.

10997 ↗

(On Diff #214533)

Why is this C++ only? C has an xor macro that comes from iso646.h. Should add a test case for this situation.

[xbolva00]

Addressed review notes

[xbolva00]

include/clang/Basic/DiagnosticGroups.td

508 ↗

(On Diff #205692)

I will inform them after patch approval.

[xbolva00]

Any futher comments or is it OK now @jfb ? @aaron.ballman ?

[aaron.ballman]

This LGTM aside from a few nits, but please give a chance for other reviewers to weigh in on their comments.

lib/Sema/SemaExpr.cpp

11025–11031 ↗

(On Diff #214534)

Might as well combine these.

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

100–102 ↗

(On Diff #214534)

I'd also like to see a case like res = 10_xor ^ 5;

[xbolva00]

Fixed nits.

[xbolva00]

Better comparison for "xor".

[hvdijk]

It's bad enough that this warns for

#define A 2
int f() { return A ^ 1; }

where as far as the users of A are concerned, A is just some arbitrary value, it's just random chance it happens to be two and there is no chance of it being misinterpreted as exponentiation, but it's much worse that this cannot be worked around by changing the definition of A to 0x2, the only way to suppress the warning with a hexadecimal constant is by explicitly using that constant in the use, i.e. by changing the function to int f() { return 0x2 ^ 1; }, which is very much inappropriate when the users of A are not supposed to care about which specific value it has.

I'd like to see this changed so that at least the #define A 0x2 case no longer triggers the warning, but ideally I'd like to see no warning for this example in its current state either. I see in the test case that that warning is intentional, but I'm not seeing from the previous discussion whether this case was based on real world programmer errors or not. Do you recall? If it was, then fair enough, I'll propose to leave that as it is.

(Edit: D97445 changed it so that the concrete example I used before no longer warns; I've edited my comment to use a different example.)

[• Quuxplusone]

In D63423#2732152, @hvdijk wrote:

It's bad enough that this warns for

#define A 2
int f() { return A ^ 1; }

where as far as the users of A are concerned...

I see how this warning is arguably overzealous in the very special case of "raising" to the constant 1 (since nobody would ever write that by accident). However, if the user of A wants to indicate that they understand this is bitwise-xor, they can simply change the body of their f to return A ^ 0x1; — hex notation suppresses the warning. (Changing the definition of A as well is perhaps a good idea but not technically required.) IMO this is good enough and we should leave it. (What do you think, having seen the A ^ 0x1 workaround? Does that sufficiently address your needs?)

[...] I'm not seeing from the previous discussion whether this case was based on real world programmer errors or not

The description links to a couple of tweets showing examples from the wild:
https://twitter.com/jfbastien/status/1139298419988549632
https://twitter.com/mikemx7f/status/1139335901790625793

[hvdijk]

In D63423#2732158, @Quuxplusone wrote:

In D63423#2732152, @hvdijk wrote:

It's bad enough that this warns for

#define A 2
int f() { return A ^ 1; }

where as far as the users of A are concerned...

I see how this warning is arguably overzealous in the very special case of "raising" to the constant 1 (since nobody would ever write that by accident). However, if the user of A wants to indicate that they understand this is bitwise-xor, they can simply change the body of their f to return A ^ 0x1; — hex notation suppresses the warning. (Changing the definition of A as well is perhaps a good idea but not technically required.) IMO this is good enough and we should leave it. (What do you think, having seen the A ^ 0x1 workaround? Does that sufficiently address your needs?)

I missed that hex notation for the RHS also suppresses the warning. Thanks, that's good to know. Combined with the fact that the case where LHS and RHS are both macros now no longer triggers the warning, that means one of LHS or RHS must be an integer literal, so there will always be a viable workaround once clang 13 is released. That may be good enough.

[...] I'm not seeing from the previous discussion whether this case was based on real world programmer errors or not

The description links to a couple of tweets showing examples from the wild:
https://twitter.com/jfbastien/status/1139298419988549632
https://twitter.com/mikemx7f/status/1139335901790625793

Those are different cases, they literally have 2 ^ in there. In the case I was asking about, the 2 comes from a macro expansion, but the ^ does not. That is much less likely to be a mistake, and one where I didn't see whether the warning for that was based on real-world concerns.

[xbolva00]

I remember that was interest to support macros too :) tbh I cant remember now such real world case where “macro ^ cst” was an issue but.. it was a long time ago ;)

If you are want, you can send patch to to avoid warning in this case, we can discuss it.

[hvdijk]

In D63423#2732186, @xbolva00 wrote:

I remember that was interest to support macros too :) tbh I cant remember now such real world case where “macro ^ cst” was an issue but.. it was a long time ago ;)

If you are want, you can send patch to to avoid warning in this case, we can discuss it.

Will do; the change is trivial after D97445: it's just changing that to use || rather than &&. I'll do that and update the tests to match. I don't know whether that'll be what we want, but it may be easier to talk about once we have a good picture from the test case update what it affects.

[xbolva00]

No, we want to preserve warning for 2 ^ MACRO or 10 ^ MACRO.

[hvdijk]

In D63423#2732199, @xbolva00 wrote:

No, we want to preserve warning for 2 ^ MACRO or 10 ^ MACRO.

Perhaps that should warn even if the RHS is in hex form, or is an enumerator constant, or is not even constant at all.

libpng: #if-ed out pseudocode but just waiting for someone to copy and paste: for (i=11;i>=0;--i){ print i, " ", (1 - e(-(2^i)/65536*l(2))) * 2^(32-i), "\n"}.

https://github.com/mongodb/mongo/blob/master/src/third_party/IntelRDFPMathLib20U1/LIBRARY/float128/mphoc_macros.h:

#define MPHOC_MAX_CHAR  ((2 ^ (BITS_PER_CHAR - 1)) - 1)
#define MPHOC_MAX_SHORT ((2 ^ (BITS_PER_SHORT - 1)) - 1)
#define MPHOC_MAX_INT   ((2 ^ (BITS_PER_INT - 1)) - 1)
#define MPHOC_MAX_LONG  ((2 ^ (BITS_PER_LONG - 1)) - 1)
#define MPHOC_MAX_WORD  ((2 ^ (BITS_PER_WORD - 1)) - 1)

(and many more in there)

[xbolva00]

Perhaps that should warn even if the RHS is in hex form

It would be kinda strange, since in one clang release we ask users to silence warning with hex form and newer release would warn anyway. Not a fan of this decision.

, or is an enumerator constant, or

This looks like a good idea. +1.

is not even constant at all.

Depends, needs to be carefully evaluated (true positives vs false positives ratio).

[hvdijk]

In D63423#2732209, @xbolva00 wrote:

Perhaps that should warn even if the RHS is in hex form

It would be kinda strange, since in one clang release we ask users to silence warning with hex form and newer release would warn anyway. Not a fan of this decision.

Fair point, but the clang suggestion is always to turn the LHS into hex form, never to turn the RHS in hex form, so if users followed clang's suggested silencing, it would continue to work. That's how I didn't even notice that I could change the RHS to hex form in the case I asked about.

And fully agreed that if we warn about any cases where we didn't warn before, we need to be very careful about the risk of false positives. When I create the new RFC patch, I'll try to get details on that and include it in the description.

[xbolva00]

When I create the new RFC patch, I'll try to get details on that and include it in the description.

Great! Thanks.

cfe/trunk/include/clang/Basic/DiagnosticGroups.td

def TautologicalCompare : DiagGroup<"tautological-compare",
                                      TautologicalUndefinedCompare,
                                      TautologicalObjCBoolCompare]>;
 def HeaderHygiene : DiagGroup<"header-hygiene">;
 def DuplicateDeclSpecifier : DiagGroup<"duplicate-decl-specifier">;
 def CompareDistinctPointerType : DiagGroup<"compare-distinct-pointer-types">;
 def GNUUnionCast : DiagGroup<"gnu-union-cast">;
 def GNUVariableSizedTypeNotAtEnd : DiagGroup<"gnu-variable-sized-type-not-at-end">;
 def Varargs : DiagGroup<"varargs">;
+def XorUsedAsPow : DiagGroup<"xor-used-as-pow">;
 
 def Unsequenced : DiagGroup<"unsequenced">;
 // GCC name for -Wunsequenced
 def : DiagGroup<"sequence-point", [Unsequenced]>;
 
 // Preprocessor warnings.
 def AmbiguousMacro : DiagGroup<"ambiguous-macro">;
 def KeywordAsMacro : DiagGroup<"keyword-macro">;

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_this_bool_conversion : Warning<
   "'this' pointer cannot be null in well-defined C++ code; pointer may be "
   "assumed to always convert to true">, InGroup<UndefinedBoolConversion>;
 def warn_address_of_reference_bool_conversion : Warning<
   "reference cannot be bound to dereferenced null pointer in well-defined C++ "
   "code; pointer may be assumed to always convert to true">,
   InGroup<UndefinedBoolConversion>;
 
+def warn_xor_used_as_pow_base_extra : Warning<
+  "result of '%0' is %1; did you mean '%2' (%3)?">,
+  InGroup<XorUsedAsPow>;
+def warn_xor_used_as_pow_base : Warning<
+  "result of '%0' is %1; did you mean '%2'?">,
+  InGroup<XorUsedAsPow>;
+def note_xor_used_as_pow_silence : Note<
+  "replace expression with '%0' to silence this warning">;
+
 def warn_null_pointer_compare : Warning<
     "comparison of %select{address of|function|array}0 '%1' %select{not |}2"
     "equal to a null pointer is always %select{true|false}2">,
     InGroup<TautologicalPointerCompare>;
 def warn_nonnull_expr_compare : Warning<
     "comparison of nonnull %select{function call|parameter}0 '%1' "
     "%select{not |}2equal to a null pointer is '%select{true|false}2' on first "
     "encounter">,

cfe/trunk/lib/Sema/SemaExpr.cpp

if (BinaryOperator::isEqualityOp(Opc) &&
     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())
if (LHS.get()->getType()->hasIntegerRepresentation() &&
                         /*AllowBothBool*/true,
                         /*AllowBoolConversions*/getLangOpts().ZVector);
     return InvalidOperands(Loc, LHS, RHS);
   }
 
   if (Opc == BO_And)
     diagnoseLogicalNotOnLHSofCheck(*this, LHS, RHS, Loc, Opc);
 
+  if (Opc == BO_Xor)
+    diagnoseXorMisusedAsPow(*this, LHS, RHS, Loc);
+
   ExprResult LHSResult = LHS, RHSResult = RHS;
   QualType compType = UsualArithmeticConversions(LHSResult, RHSResult,
                                                  IsCompAssign);
   if (LHSResult.isInvalid() || RHSResult.isInvalid())
     return QualType();
   LHS = LHSResult.get();
   RHS = RHSResult.get();
 
ExprResult Sema::ActOnObjCAvailabilityCheckExpr(
   // warn when it's used inappropriately (i.e. not if(@available)).
   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

cfe/trunk/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 TWO_ULL 2ULL
+#define EPSILON 10 ^ -300
+
+#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
+#endif
+
+void test(unsigned a, unsigned b) {
+  unsigned res;
+  res = a ^ 5;
+  res = 2 ^ b;
+  res = a ^ b;
+  res = 2 ^ -1;
+  res = 2 ^ 0; // expected-warning {{result of '2 ^ 0' is 2; did you mean '1 << 0' (1)?}}
+               // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:14}:"1"
+               // expected-note@-2 {{replace expression with '0x2 ^ 0' to silence this warning}}
+  res = 2 ^ 1; // expected-warning {{result of '2 ^ 1' is 3; did you mean '1 << 1' (2)?}}
+               // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:14}:"1 << 1"
+               // expected-note@-2 {{replace expression with '0x2 ^ 1' to silence this warning}}
+  res = 2 ^ 2; // expected-warning {{result of '2 ^ 2' is 0; did you mean '1 << 2' (4)?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:14}:"1 << 2"
+  // expected-note@-2 {{replace expression with '0x2 ^ 2' to silence this warning}}
+  res = 2 ^ 8; // expected-warning {{result of '2 ^ 8' is 10; did you mean '1 << 8' (256)?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:14}:"1 << 8"
+  // expected-note@-2 {{replace expression with '0x2 ^ 8' to silence this warning}}
+  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 ^ 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 = 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);
+  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 = EPSILON;
+  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 ^ 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
+}