[PATCH] D63423: [Diagnostics] Diagnose misused xor as pow
Arthur O'Dwyer via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 17 11:07:03 PDT 2019
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
More information about the cfe-commits
mailing list