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

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

Quuxplusone added inline comments.


================
Comment at: lib/Sema/SemaExpr.cpp:10931
+  // Do not diagnose hexadecimal literals
+  if (ExprStr.find("0x") != llvm::StringRef::npos)
+    return;
----------------
Can you use `starts_with` (or the LLVM equivalent) in both of these cases? It'll be faster and also more correct.

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


================
Comment at: lib/Sema/SemaExpr.cpp:10982
+    S.Diag(Loc, diag::note_xor_used_as_pow_silence) << ("0x2 ^ " + RHSStr);
+  } else if (LeftSideValue == 10 && RightSideIntValue != 0) {
+    std::string SuggestedValue;
----------------
Suppressing the warning specifically on `10 ^ 0` (which means `10`) seems like an anti-feature.


================
Comment at: lib/Sema/SemaExpr.cpp:10988
+    } else {
+      SuggestedValue = "1" + std::string(RightSideIntValue, '0');
+    }
----------------
I suggest at least one unit test case that involves `10 ^ 100`, and considering the user-friendliness of the resulting error message. How about suggesting `"1e" + std::to_string(RightSideIntValue)` as the fixit in both cases?


================
Comment at: test/SemaCXX/warn-xor-as-pow.cpp:52
+  res = FOOBAR(2, 16);
+  res = EPSILON;
+  res = 0b10 ^ 16;
----------------
I still expect to see a warning either on this line, or on the line where the macro is defined. I don't see why we'd want to be silent in this case.


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

https://reviews.llvm.org/D63423