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

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

Reply via email to