[PATCH] D136532: [clang][Interp] Implement left and right shifts

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/Interp/Interp.h:1291-1305
+  if (RHS.isNegative()) {
     const SourceInfo &Loc = S.Current->getSource(OpPC);
     S.CCEDiag(Loc, diag::note_constexpr_negative_shift) << RHS.toAPSInt();
-    return ShiftLeft<TL, TR>(S, OpPC, LHS, Trunc<TR, TL>(S, OpPC, Bits, -RHS));
-  } else {
-    return ShiftRight<TL, TR>(S, OpPC, LHS, Trunc<TR, TL>(S, OpPC, Bits, RHS));
+    return false;
   }
-}
 
+  // C++11 [expr.shift]p1: Shift width must be less than the bit width of
----------------
Should we factor this logic out into a common helper since it's the same between shl and shr? (It could be done as an NFC commit after we land this, too, I don't feel super strongly about it.)


================
Comment at: clang/test/AST/Interp/shifts.cpp:6
+
+#define CHAR_BIT (sizeof(char) * 8)
+#define WORD_BIT (sizeof(int) * 8)
----------------
You can get rid of this entirely and use `__CHAR_BIT__` instead.


================
Comment at: clang/test/AST/Interp/shifts.cpp:7
+#define CHAR_BIT (sizeof(char) * 8)
+#define WORD_BIT (sizeof(int) * 8)
+#define INT_MAX (__INT_MAX__)
----------------
You can get rid of this entirely and use `__INT_WIDTH__` instead.


================
Comment at: clang/test/AST/Interp/shifts.cpp:8
+#define WORD_BIT (sizeof(int) * 8)
+#define INT_MAX (__INT_MAX__)
+#define INT_MIN (~__INT_MAX__)
----------------
No need for this define at all, but fine to leave if you want it for parity with `INT_MIN`.


================
Comment at: clang/test/AST/Interp/shifts.cpp:116
+  static_assert(true << 1, "");
+  static_assert(1 << (WORD_BIT +1) == 0, "");  // expected-error {{not an integral constant expression}} \
+                                               // expected-note {{>= width of type 'int'}} \
----------------
I'd also appreciate tests showing:
```
constexpr int i1 = 1 << -1; // ill-formed due to UB
constexpr int i2 = 1 << (WORD_BIT + 1); // ill-formed due to UB
constexpr char c = 1;
constexpr int i3 = c << (CHAR_BIT + 1); // Not ill-formed
```


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

https://reviews.llvm.org/D136532