[PATCH] D63423: [Diagnostics] Diagnose misused xor as pow

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

Quuxplusone added a comment.

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

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

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



================
Comment at: lib/Sema/SemaExpr.cpp:10913
+  if (ExprStr.find("0b") != llvm::StringRef::npos)
+    return;
+  if (S.getLangOpts().CPlusPlus) {
----------------
Why do you special-case `0b` prefix (or suffix — `0x0b` is also special-cased by this snippet), but you don't special-case octal or hexadecimal constants? I'm sure `x ^ 0x1234` will be a more common spelling than `x ^ 0b1001000110100`.


================
Comment at: lib/Sema/SemaExpr.cpp:10924
+
+  if (LeftSideValue == 2 || LeftSideValue == 10) {
+    llvm::APInt XorValue = LeftSideValue ^ RightSideValue;
----------------
Do you have metrics indicating that this line is an improvement over `if (true) {`?


================
Comment at: lib/Sema/SemaExpr.cpp:10959
+    S.Diag(Loc, diag::note_xor_used_as_pow_silence)
+        << ("(" + LHSStr + ") ^ " + RHSStr);
+  }
----------------
I don't understand why parenthesizing one argument should silence the warning. Wouldn't it be more natural to suggest converting both arguments to hexadecimal or binary?  That is, convert `10 ^ x` to `0xA ^ x`, or `2 ^ 16` to `0x2 ^ 0x10`.


================
Comment at: test/Sema/warn-xor-as-pow.c:38
+  res = 0b10 ^ 16;
+  res = 2 ^ 0b100;
+  res = XOR(2, 16);
----------------
Please add test cases for `2 ^ 0x4` and `2 ^ 04` and `0x2 ^ 10` and `02 ^ 10`.


================
Comment at: test/Sema/warn-xor-as-pow.c:39
+  res = 2 ^ 0b100;
+  res = XOR(2, 16);
+  unsigned char two = 2;
----------------
I don't understand why this line doesn't warn. Is it because the macro's name has the case-insensitive string `xor` in it? Is it because there is a macro involved at all? In either case, please add another test case for `res = FOOBAR(2, 16)`.

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


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

https://reviews.llvm.org/D63423