tbaeder added inline comments.

================
Comment at: clang/lib/AST/Interp/Interp.h:1246-1256
   if (V.isSigned() && !S.getLangOpts().CPlusPlus20) {
     // C++11 [expr.shift]p2: A signed left shift must have a non-negative
     // operand, and must not overflow the corresponding unsigned type.
     // C++2a [expr.shift]p2: E1 << E2 is the unique value congruent to
     // E1 x 2^E2 module 2^N.
     if (V.isNegative()) {
       const Expr *E = S.Current->getExpr(OpPC);
----------------
aaron.ballman wrote:
> Why do `Shr` and `Shl` check for a negative shift amount to issue a 
> diagnostic but we check for signed in `ShiftLeft` to issue a diagnostic? (I 
> would have expected the checks to all live in the same place.)
This was already implemented and I tried to keep as much code as possible, but 
I'll change that strategy ;)


================
Comment at: clang/test/AST/Interp/shifts.cpp:57
+    //c >>= 999999; // expected-warning {{shift count >= width of type}}
+    //c <<= CHAR_BIT; // expected-warning {{shift count >= width of type}}
+    //c >>= CHAR_BIT; // expected-warning {{shift count >= width of type}}
----------------
shafik wrote:
> This is not correct, the operands go through integral promotions first and 
> the result is the promoted type of the left operand see [dcl.shift 
> p1](https://eel.is/c++draft/expr.shift#1).
> 
> Also see godbolt: https://godbolt.org/z/7qzKjojMb
Hmm, this is copy-pasted from `test/SemaCXX/shift.cpp`.


================
Comment at: clang/test/AST/Interp/shifts.cpp:70
+    i = 1 << (WORD_BIT - 1); // cxx17-warning-not {{sets the sign bit of the 
shift expression}}
+    i = -1 << (WORD_BIT - 1); // cxx17-warning {{shifting a negative signed 
value is undefined}} \
+                              // ref-cxx17-warning {{shifting a negative 
signed value is undefined}}
----------------
shafik wrote:
> shafik wrote:
> > A negative left operand was made well-formed in C++20 I believe see 
> > godbolt: https://godbolt.org/z/7qzKjojMb
> > 
> > My reference from above for `expr.shift/p1` also applies. 
> > 
> > Although a negative right operand is still UB.
> > 
> > Also note shifting into the sign bit was made well-formed in C++11: 
> > https://stackoverflow.com/questions/19593938/is-left-shifting-a-negative-integer-undefined-behavior-in-c11#comment29091986_19593938
> Typo, shifting into the sign bit was made well-formed after C++11
This is in line with the test, isn't it? The warning is only for `cxx17`, the 
c++20 tests don't expect any output.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136532

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

Reply via email to