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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
