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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 marked an inline comment as done.
xbolva00 added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:11067
+
+  // Do not diagnose 2 ^ 64, but allow special case (2 ^ 64) - 1.
+  if (SubLHS && SubRHS && (LeftSideValue != 2 || RightSideValue != 64))

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

Maybe I should just revert it..


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:11067
+
+  // Do not diagnose 2 ^ 64, but allow special case (2 ^ 64) - 1.
+  if (SubLHS && SubRHS && (LeftSideValue != 2 || RightSideValue != 64))

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Ok, I will fix it as you suggested.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:11067
+
+  // Do not diagnose 2 ^ 64, but allow special case (2 ^ 64) - 1.
+  if (SubLHS && SubRHS && (LeftSideValue != 2 || RightSideValue != 64))

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

Agreed with this. We can just leave the fix off in the circumstances it's not 
correct while still warning that the original code is suspicious.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 marked an inline comment as done.
xbolva00 added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:11067
+
+  // Do not diagnose 2 ^ 64, but allow special case (2 ^ 64) - 1.
+  if (SubLHS && SubRHS && (LeftSideValue != 2 || RightSideValue != 64))

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:11067
+
+  // Do not diagnose 2 ^ 64, but allow special case (2 ^ 64) - 1.
+  if (SubLHS && SubRHS && (LeftSideValue != 2 || RightSideValue != 64))

xbolva00 wrote:
> xbolva00 wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > This comment explains what the code does, but not why it does it. Given 
> > > > that we're adding special cases, I think more comments here explaining 
> > > > why this is valuable would be appreciated.
> > > Thank you, the comments helped! But they also raised another question for 
> > > me. What's special about 2^64? Why not (2^16) - 1 or other common 
> > > power-of-two values? I would have expected 8, 16, 32, and 64 to be 
> > > handled similarly.
> > We generally suggest 1LL << C. But here we cant say 1LL << 64.
> > 
> > (2^16)-1 is diagnosed normally since we go here from “visit xor” code.
> >  
> > 
> > This (2^64)-1 handling was suggested by jfb.
> > 
> > I see no motivation cases to diagnose 2^65, 2^100, ...
> > 
> > 
> > 
> > 
> I think the suggestion for (2^32)-1:
> (1LL<32)-1 is good, or should we make things more complicated and suggest Int 
> max macro? 
> We generally suggest 1LL << C. But here we cant say 1LL << 64.

Ah, good point.

> I see no motivation cases to diagnose 2^65, 2^100, ...

Me neither, I was more wondering about the common powers of two.

> I think the suggestion for (2^32)-1:
> (1LL<32)-1 is good, or should we make things more complicated and suggest Int 
> max macro?

I feel like this is somewhat clang-tidy territory more than the compiler 
properly, including the 2^64 - 1 case, because it is likely to be very uncommon 
due to how specific it is. However, given that this only triggers on xor and 
only with integer literals, it shouldn't cause too much of a compilation 
slow-down in general to do it here.

I tend to err on the side of consistency, so my feeling is that if we want the 
64 case to suggest ULLONG, we'd want the other cases to behave similarly. 
Alternatively, rather than handling this specific issue in the compiler, handle 
it in a `bugprone` clang-tidy check where we can also give the user more 
control over how they want to correct their mistake (e.g., 
`std::numeric_limits::max()` vs `LONG_MAX` vs `~0L`).


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 marked an inline comment as done.
xbolva00 added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:11067
+
+  // Do not diagnose 2 ^ 64, but allow special case (2 ^ 64) - 1.
+  if (SubLHS && SubRHS && (LeftSideValue != 2 || RightSideValue != 64))

xbolva00 wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > This comment explains what the code does, but not why it does it. Given 
> > > that we're adding special cases, I think more comments here explaining 
> > > why this is valuable would be appreciated.
> > Thank you, the comments helped! But they also raised another question for 
> > me. What's special about 2^64? Why not (2^16) - 1 or other common 
> > power-of-two values? I would have expected 8, 16, 32, and 64 to be handled 
> > similarly.
> We generally suggest 1LL << C. But here we cant say 1LL << 64.
> 
> (2^16)-1 is diagnosed normally since we go here from “visit xor” code.
>  
> 
> This (2^64)-1 handling was suggested by jfb.
> 
> I see no motivation cases to diagnose 2^65, 2^100, ...
> 
> 
> 
> 
I think the suggestion for (2^32)-1:
(1LL<32)-1 is good, or should we make things more complicated and suggest Int 
max macro? 


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 marked an inline comment as done.
xbolva00 added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:11067
+
+  // Do not diagnose 2 ^ 64, but allow special case (2 ^ 64) - 1.
+  if (SubLHS && SubRHS && (LeftSideValue != 2 || RightSideValue != 64))

aaron.ballman wrote:
> aaron.ballman wrote:
> > This comment explains what the code does, but not why it does it. Given 
> > that we're adding special cases, I think more comments here explaining why 
> > this is valuable would be appreciated.
> Thank you, the comments helped! But they also raised another question for me. 
> What's special about 2^64? Why not (2^16) - 1 or other common power-of-two 
> values? I would have expected 8, 16, 32, and 64 to be handled similarly.
We generally suggest 1LL << C. But here we cant say 1LL << 64.

(2^16)-1 is diagnosed normally since we go here from “visit xor” code.
 

This (2^64)-1 handling was suggested by jfb.

I see no motivation cases to diagnose 2^65, 2^100, ...






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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:11067
+
+  // Do not diagnose 2 ^ 64, but allow special case (2 ^ 64) - 1.
+  if (SubLHS && SubRHS && (LeftSideValue != 2 || RightSideValue != 64))

aaron.ballman wrote:
> This comment explains what the code does, but not why it does it. Given that 
> we're adding special cases, I think more comments here explaining why this is 
> valuable would be appreciated.
Thank you, the comments helped! But they also raised another question for me. 
What's special about 2^64? Why not (2^16) - 1 or other common power-of-two 
values? I would have expected 8, 16, 32, and 64 to be handled similarly.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 marked an inline comment as done.
xbolva00 added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:11031-11032
   // Do not diagnose macros.
-  if (Loc.isMacroID())
+  if (Loc.isMacroID() || XorLHS.get()->getBeginLoc().isMacroID() ||
+  XorRHS.get()->getBeginLoc().isMacroID())
 return;

aaron.ballman wrote:
> 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.
+1, agree.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 218258.
xbolva00 added a comment.

Fixed review comments


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

https://reviews.llvm.org/D66397

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/SemaCXX/warn-xor-as-pow.cpp

Index: test/SemaCXX/warn-xor-as-pow.cpp
===
--- test/SemaCXX/warn-xor-as-pow.cpp
+++ test/SemaCXX/warn-xor-as-pow.cpp
@@ -10,8 +10,10 @@
 #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
 
@@ -21,7 +23,7 @@
 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) {
@@ -32,25 +34,31 @@
   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}}
+   // expected-note@-2 {{replace expression with '0x2 ^ 0' or use 'xor' instead of '^' 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}}
+   // expected-note@-2 {{replace expression with '0x2 ^ 1' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 2' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 8' or use 'xor' instead of '^' 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]]:15}:"1 << +8"
+  // expected-note@-2 {{replace expression with '0x2 ^ +8' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 8' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 16' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ TEN' or use 'xor' instead of '^' 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' or use 'xor' instead of '^' to silence this warning}}
   res = 0x2 ^ 16;
   res = 2 xor 16;
 
@@ -69,31 +77,56 @@
   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;
+  // expected-note@-2 {{replace expression with '0x2 ^ 32' or use 'xor' instead of '^' to silence this warning}}
+  res = (2 ^ 64) - 1; // expected-warning {{result of '2 ^ 64' is 66; did you mean '~0ULL'?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:21}:"~0ULL"
+  // expected-note@-2 {{replace expression with '0x2 ^ 64' or use 'xor' instead of '^' to silence this warning}}
+#define ULLONG_MAX 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:54-55
+const SourceLocation Loc,
+const Expr *SubLHS = nullptr,
+const Expr *SubRHS = nullptr);
+

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



Comment at: lib/Sema/SemaExpr.cpp:11031-11032
   // Do not diagnose macros.
-  if (Loc.isMacroID())
+  if (Loc.isMacroID() || XorLHS.get()->getBeginLoc().isMacroID() ||
+  XorRHS.get()->getBeginLoc().isMacroID())
 return;

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.



Comment at: lib/Sema/SemaExpr.cpp:11052
+  Negative = (Opc == UO_Minus);
+  ExplicitPlus = (Opc == UO_Plus);
 } else {

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



Comment at: lib/Sema/SemaExpr.cpp:11067
+
+  // Do not diagnose 2 ^ 64, but allow special case (2 ^ 64) - 1.
+  if (SubLHS && SubRHS && (LeftSideValue != 2 || RightSideValue != 64))

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.



Comment at: lib/Sema/SemaExpr.cpp:11073
   LHSInt->getBeginLoc(), S.getLocForEndOfToken(RHSInt->getLocation()));
-  llvm::StringRef ExprStr =
+  std::string ExprStr =
   Lexer::getSourceText(ExprRange, S.getSourceManager(), S.getLangOpts());

Why is this type changing?



Comment at: lib/Sema/SemaExpr.cpp:11078
   CharSourceRange::getCharRange(Loc, S.getLocForEndOfToken(Loc));
-  llvm::StringRef XorStr =
+  std::string XorStr =
   Lexer::getSourceText(XorRange, S.getSourceManager(), S.getLangOpts());

Same question here.



Comment at: lib/Sema/SemaExpr.cpp:11130
+  ? "ULLONG_MAX"
+  : "-1LL";
+  ExprRange = CharSourceRange::getCharRange(

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 218115.
xbolva00 added a comment.

Fixed review comment.


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

https://reviews.llvm.org/D66397

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/SemaCXX/warn-xor-as-pow.cpp

Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -,7 +,7 @@
   "result of '%0' is %1; did you mean '%2'?">,
   InGroup;
 def note_xor_used_as_pow_silence : Note<
-  "replace expression with '%0' to silence this warning">;
+  "replace expression with '%0' %select{|or use 'xor' instead of '^' }1to silence this warning">;
 
 def warn_null_pointer_compare : Warning<
 "comparison of %select{address of|function|array}0 '%1' %select{not |}2"
Index: test/SemaCXX/warn-xor-as-pow.cpp
===
--- test/SemaCXX/warn-xor-as-pow.cpp
+++ test/SemaCXX/warn-xor-as-pow.cpp
@@ -10,8 +10,10 @@
 #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
 
@@ -21,7 +23,7 @@
 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) {
@@ -32,25 +34,25 @@
   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}}
+   // expected-note@-2 {{replace expression with '0x2 ^ 0' or use 'xor' instead of '^' 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}}
+   // expected-note@-2 {{replace expression with '0x2 ^ 1' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 2' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 8' or use 'xor' instead of '^' 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]]:15}:"1 << +8"
+  // expected-note@-2 {{replace expression with '0x2 ^ +8' or use 'xor' instead of '^' to silence this warning}}
+  res = TWO ^ 8;
   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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 16' or use 'xor' instead of '^' to silence this warning}}
+  res = 2 ^ TEN;
+  res = res + (2 ^ ALPHA_OFFSET);
   res = 0x2 ^ 16;
   res = 2 xor 16;
 
@@ -69,31 +71,50 @@
   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;
+  // expected-note@-2 {{replace expression with '0x2 ^ 32' or use 'xor' instead of '^' to silence this warning}}
+  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 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: rsmith.
aaron.ballman added a comment.

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a subscriber: regehr.
xbolva00 added a comment.

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

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 216724.

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

https://reviews.llvm.org/D66397

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/SemaCXX/warn-xor-as-pow.cpp

Index: test/SemaCXX/warn-xor-as-pow.cpp
===
--- test/SemaCXX/warn-xor-as-pow.cpp
+++ test/SemaCXX/warn-xor-as-pow.cpp
@@ -10,8 +10,10 @@
 #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
 
@@ -21,7 +23,7 @@
 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) {
@@ -32,25 +34,25 @@
   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}}
+   // expected-note@-2 {{replace expression with '0x2 ^ 0' or use 'xor' instead of '^' 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}}
+   // expected-note@-2 {{replace expression with '0x2 ^ 1' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 2' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 8' or use 'xor' instead of '^' 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]]:15}:"1 << +8"
+  // expected-note@-2 {{replace expression with '0x2 ^ +8' or use 'xor' instead of '^' to silence this warning}}
+  res = TWO ^ 8;
   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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 16' or use 'xor' instead of '^' to silence this warning}}
+  res = 2 ^ TEN;
+  res = res + (2 ^ ALPHA_OFFSET);
   res = 0x2 ^ 16;
   res = 2 xor 16;
 
@@ -69,31 +71,50 @@
   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;
+  // expected-note@-2 {{replace expression with '0x2 ^ 32' or use 'xor' instead of '^' to silence this warning}}
+  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' or use 'xor' instead of '^' 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' or use 'xor' instead of '^' to silence this warning}}
+  res = (2 ^ 64) - 2;
+  res = (2 ^ IOP) - 1;
+  res = (2 ^ 64) - 1u; // expected-warning {{result of '2 ^ 64' is 66; did you mean 'ULLONG_MAX'?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:22}:"ULLONG_MAX"
+  // expected-note@-2 

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 216723.

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

https://reviews.llvm.org/D66397

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/SemaCXX/warn-xor-as-pow.cpp

Index: test/SemaCXX/warn-xor-as-pow.cpp
===
--- test/SemaCXX/warn-xor-as-pow.cpp
+++ test/SemaCXX/warn-xor-as-pow.cpp
@@ -10,8 +10,10 @@
 #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
 
@@ -21,7 +23,7 @@
 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) {
@@ -32,25 +34,25 @@
   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}}
+   // expected-note@-2 {{replace expression with '0x2 ^ 0' or use 'xor' instead of '^' 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}}
+   // expected-note@-2 {{replace expression with '0x2 ^ 1' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 2' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 8' or use 'xor' instead of '^' 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]]:15}:"1 << +8"
+  // expected-note@-2 {{replace expression with '0x2 ^ +8' or use 'xor' instead of '^' to silence this warning}}
+  res = TWO ^ 8;
   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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 16' or use 'xor' instead of '^' to silence this warning}}
+  res = 2 ^ TEN;
+  res = res + (2 ^ ALPHA_OFFSET);
   res = 0x2 ^ 16;
   res = 2 xor 16;
 
@@ -69,31 +71,50 @@
   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;
+  // expected-note@-2 {{replace expression with '0x2 ^ 32' or use 'xor' instead of '^' to silence this warning}}
+  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' or use 'xor' instead of '^' 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' or use 'xor' instead of '^' to silence this warning}}
+  res = (2 ^ 64) - 2;
+  res = (2 ^ IOP) - 1;
+  res = (2 ^ 64) - 1u; // expected-warning {{result of '2 ^ 64' is 66; did you mean 'ULLONG_MAX'?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:22}:"ULLONG_MAX"
+  // expected-note@-2 

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 216720.
xbolva00 added a comment.

Addressed Bruno Ricci's tips.
Removed macro support.


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

https://reviews.llvm.org/D66397

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/SemaCXX/warn-xor-as-pow.cpp

Index: test/SemaCXX/warn-xor-as-pow.cpp
===
--- test/SemaCXX/warn-xor-as-pow.cpp
+++ test/SemaCXX/warn-xor-as-pow.cpp
@@ -10,8 +10,10 @@
 #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
 
@@ -21,7 +23,7 @@
 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) {
@@ -32,25 +34,25 @@
   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}}
+   // expected-note@-2 {{replace expression with '0x2 ^ 0' or use 'xor' instead of '^' 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}}
+   // expected-note@-2 {{replace expression with '0x2 ^ 1' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 2' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 8' or use 'xor' instead of '^' 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]]:15}:"1 << +8"
+  // expected-note@-2 {{replace expression with '0x2 ^ +8' or use 'xor' instead of '^' to silence this warning}}
+  res = TWO ^ 8;
   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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 16' or use 'xor' instead of '^' to silence this warning}}
+  res = 2 ^ TEN;
+  res = res + (2 ^ ALPHA_OFFSET);
   res = 0x2 ^ 16;
   res = 2 xor 16;
 
@@ -69,31 +71,50 @@
   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;
+  // expected-note@-2 {{replace expression with '0x2 ^ 32' or use 'xor' instead of '^' to silence this warning}}
+  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' or use 'xor' instead of '^' 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' or use 'xor' instead of '^' to silence this warning}}
+  res = (2 ^ 64) - 2;
+  res = (2 ^ IOP) - 1;
+  res = (2 ^ 64) - 1u; // expected-warning {{result of '2 ^ 64' is 66; did you mean 'ULLONG_MAX'?}}
+  // CHECK: 

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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

First patch 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.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

>>   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 :(


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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?


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

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

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.




Comment at: lib/Sema/SemaExpr.cpp:9449
+  }
+
+  if (LHSInt->getValue().getBitWidth() != RHSInt->getValue().getBitWidth())

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  ?



Comment at: lib/Sema/SemaExpr.cpp:9469
+  const llvm::APInt XorValue = LeftSideValue ^ RightSideValue;
+
+  std::string LHSStr = Lexer::getSourceText(

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



Comment at: lib/Sema/SemaExpr.cpp:9506
+return;
+
+  bool SuggestXor = S.getLangOpts().CPlusPlus || 
S.getPreprocessor().isMacroDefined("xor");

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



Comment at: lib/Sema/SemaExpr.cpp:9674
+RHS.get());
+
   // Enforce type constraints: C99 6.5.6p3.

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` ?


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

I think everybody should be fine with this now :)


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Implemented @Quuxplusone's idea to improve silence note.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 216478.

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

https://reviews.llvm.org/D66397

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/SemaCXX/warn-xor-as-pow.cpp

Index: test/SemaCXX/warn-xor-as-pow.cpp
===
--- test/SemaCXX/warn-xor-as-pow.cpp
+++ test/SemaCXX/warn-xor-as-pow.cpp
@@ -10,8 +10,10 @@
 #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
 
@@ -21,7 +23,7 @@
 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) {
@@ -32,25 +34,28 @@
   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}}
+   // expected-note@-2 {{replace expression with '0x2 ^ 0' or use 'xor' instead of '^' 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}}
+   // expected-note@-2 {{replace expression with '0x2 ^ 1' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 2' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 8' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 8' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ 16' or use 'xor' instead of '^' 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}}
+  // expected-note@-2 {{replace expression with '0x2 ^ TEN' or use 'xor' instead of '^' 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' or use 'xor' instead of '^' to silence this warning}}
   res = 0x2 ^ 16;
   res = 2 xor 16;
 
@@ -63,37 +68,59 @@
   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' or use 'xor' instead of '^' 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 = EPSILON;
+  // expected-note@-2 {{replace expression with '0x2 ^ 32' or use 'xor' instead of '^' to silence this warning}}
+  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' or use 'xor' instead of '^' to silence this warning}}

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 216458.
xbolva00 edited the summary of this revision.
xbolva00 added a comment.

Detect digit separators.


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

https://reviews.llvm.org/D66397

Files:
  lib/Sema/SemaExpr.cpp
  test/SemaCXX/warn-xor-as-pow.cpp

Index: test/SemaCXX/warn-xor-as-pow.cpp
===
--- test/SemaCXX/warn-xor-as-pow.cpp
+++ test/SemaCXX/warn-xor-as-pow.cpp
@@ -3,6 +3,7 @@
 // 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++ -std=c++14 -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
 
@@ -10,8 +11,10 @@
 #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
 
@@ -21,7 +24,7 @@
 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) {
@@ -51,6 +54,9 @@
   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;
 
@@ -63,16 +69,35 @@
   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}}
@@ -91,6 +116,9 @@
   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 

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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”


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

>> C++14 binary literals

Thanks, noted.

>> Suggest “xor”

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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?


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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?


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 216399.
xbolva00 added a comment.

Fixed review nits.


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

https://reviews.llvm.org/D66397

Files:
  lib/Sema/SemaExpr.cpp
  test/SemaCXX/warn-xor-as-pow.cpp

Index: test/SemaCXX/warn-xor-as-pow.cpp
===
--- test/SemaCXX/warn-xor-as-pow.cpp
+++ test/SemaCXX/warn-xor-as-pow.cpp
@@ -10,8 +10,10 @@
 #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
 
@@ -21,7 +23,7 @@
 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) {
@@ -51,6 +53,9 @@
   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;
 
@@ -63,16 +68,35 @@
   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}}
@@ -91,6 +115,9 @@
   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}}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -9422,6 +9422,133 @@
 << RHSExpr->getSourceRange();
 }
 
+static void diagnoseXorMisusedAsPow(Sema , const ExprResult ,
+const ExprResult ,
+const SourceLocation Loc,
+

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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 ?


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

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.




Comment at: lib/Sema/SemaExpr.cpp:9429
+const Expr *SubLHS = nullptr,
+const Expr *SubRHS = nullptr) {
+  // Do not diagnose xor in macros.

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



Comment at: lib/Sema/SemaExpr.cpp:9434
+
+  // Do not diagnose if xor's rhs is macro.
+  if (RHS.get()->getBeginLoc().isMacroID())

s/is macro/is a macro ?


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Any objections to this patch except “macro” question?


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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

No, there is a note “replace with 0x2 ^ ALPHA_OFFSET” to silence it.

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

I really dont think it is worth to do.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

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


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a subscriber: hans.
xbolva00 added a comment.

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

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

@tkanis 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.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 215957.
xbolva00 added a comment.

Do not warn if RHS is macro.


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

https://reviews.llvm.org/D66397

Files:
  lib/Sema/SemaExpr.cpp
  test/SemaCXX/warn-xor-as-pow.cpp

Index: test/SemaCXX/warn-xor-as-pow.cpp
===
--- test/SemaCXX/warn-xor-as-pow.cpp
+++ test/SemaCXX/warn-xor-as-pow.cpp
@@ -10,8 +10,10 @@
 #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
 
@@ -48,9 +50,7 @@
   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 = 2 ^ TEN;
   res = 0x2 ^ 16;
   res = 2 xor 16;
 
@@ -70,7 +70,21 @@
   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;
+  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 = 10 ^ 0; // expected-warning {{result of '10 ^ 0' is 10; did you mean '1e0'?}}
@@ -102,4 +116,5 @@
   res = 10 ^ 5_0b;
   res = 10_0X ^ 5;
 #endif
+  res = res + (2 ^ ALPHA_OFFSET);
 }
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -9422,6 +9422,133 @@
 << RHSExpr->getSourceRange();
 }
 
+static void diagnoseXorMisusedAsPow(Sema , const ExprResult ,
+const ExprResult ,
+const SourceLocation Loc,
+const Expr *SubLHS = nullptr,
+const Expr *SubRHS = nullptr) {
+  // Do not diagnose xor in macros.
+  if (Loc.isMacroID())
+return;
+
+  // Do not diagnose if xor's rhs is macro.
+  if (RHS.get()->getBeginLoc().isMacroID())
+return;
+
+  bool Negative = false;
+  const auto *LHSInt = dyn_cast(LHS.get());
+  const auto *RHSInt = dyn_cast(RHS.get());
+
+  if (!LHSInt)
+return;
+  if (!RHSInt) {
+// Check negative literals.
+if (const auto *UO = dyn_cast(RHS.get())) {
+  if (UO->getOpcode() != UO_Minus)
+return;
+  RHSInt = dyn_cast(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  = LHSInt->getValue();
+  const llvm::APInt  = 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 

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a subscriber: jfb.
xbolva00 added a comment.

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

@jfb @aaron.ballman


Repository:
  rC Clang

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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

From post commit discussion, Nico Weber:

1. Hi,
2.
3. this results in a false positive on webrtc, on this code:
4.
5. 
https://cs.chromium.org/chromium/src/third_party/libwebp/src/enc/picture_csp_enc.c?q=picture_csp_enc.c=package:chromium=1002
6.
7. The code does this:
8.
9. #ifdef WORDS_BIGENDIAN
10. #define ALPHA_OFFSET 0   // uint32_t 0xff00 is 0xff,00,00,00 in memory
11. #else
12. #define ALPHA_OFFSET 3   // uint32_t 0xff00 is 0x00,00,00,ff in memory
13. #endif
14.
15. // ...
16.
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);
22.
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.
24.
25. Maybe this shouldn't fire if the rhs is a macro?
26.
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.)




Repository:
  rC Clang

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

https://reviews.llvm.org/D66397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 created this revision.
xbolva00 added a reviewer: aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Handle specific case

(2 ^ 64) - 1


Repository:
  rC Clang

https://reviews.llvm.org/D66397

Files:
  lib/Sema/SemaExpr.cpp
  test/SemaCXX/warn-xor-as-pow.cpp

Index: test/SemaCXX/warn-xor-as-pow.cpp
===
--- test/SemaCXX/warn-xor-as-pow.cpp
+++ test/SemaCXX/warn-xor-as-pow.cpp
@@ -70,7 +70,20 @@
   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 ^ 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 = 10 ^ 0; // expected-warning {{result of '10 ^ 0' is 10; did you mean '1e0'?}}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -9422,6 +9422,129 @@
 << RHSExpr->getSourceRange();
 }
 
+static void diagnoseXorMisusedAsPow(Sema , const ExprResult ,
+const ExprResult ,
+const SourceLocation Loc,
+const Expr *SubLHS = nullptr,
+const Expr *SubRHS = nullptr) {
+  // Do not diagnose macros.
+  if (Loc.isMacroID())
+return;
+
+  bool Negative = false;
+  const auto *LHSInt = dyn_cast(LHS.get());
+  const auto *RHSInt = dyn_cast(RHS.get());
+
+  if (!LHSInt)
+return;
+  if (!RHSInt) {
+// Check negative literals.
+if (const auto *UO = dyn_cast(RHS.get())) {
+  if (UO->getOpcode() != UO_Minus)
+return;
+  RHSInt = dyn_cast(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  = LHSInt->getValue();
+  const llvm::APInt  = 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 (SubLHS && SubRHS && (LeftSideValue != 2 || RightSideIntValue != 64))
+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) {
+