erichkeane added a subscriber: craig.topper.
erichkeane added inline comments.
================
Comment at: clang/include/clang/Lex/LiteralSupport.h:65
bool isLong : 1; // This is *not* set for long long.
bool isLongLong : 1;
bool isSizeT : 1; // 1z, 1uz (C++2b)
----------------
I can't help but wonder if there are some really good opportunities to merge a
bunch of these mutually exclusive ones into an enum...
```
enum class SizeType {
Long,
LongLong,
SizeT,
Float,
Imaginary,
Float16,
Flaot128,
Fract,
Accum,
BitInt
```
Or something?
================
Comment at: clang/lib/AST/StmtPrinter.cpp:1157
+ if (const auto *BT = Node->getType()->getAs<BitIntType>()) {
+ OS << (BT->isUnsigned() ? "uwb" : "wb");
+ return;
----------------
Nit: Instead of BT->isUnsigned(), we already have isSigned above.
================
Comment at: clang/lib/Lex/LiteralSupport.cpp:903
+ break; // Invalid for floats.
+ if (HasSize)
+ break; // Invalid if we already have a size for the literal.
----------------
Do we also have to reject isImaginary/isFract/etc separately?
================
Comment at: clang/lib/Sema/SemaExpr.cpp:3987
+ << Literal.isUnsigned;
+ Width = MaxBitIntWidth;
+ }
----------------
Do you have some AST test here to see what happens to the value when the width
is exceeded?
================
Comment at: clang/lib/Sema/SemaExpr.cpp:3995
+ ResultVal = ResultVal.zextOrTrunc(Width);
+ Ty = Context.getBitIntType(Literal.isUnsigned, Width);
+ }
----------------
I think it is already covered, but do we make sure the literal itself and the
suffix match? That is: -111uwb is invalid?
================
Comment at: clang/test/AST/bitint-suffix.c:20
+ // CHECK: TypedefDecl 0x{{[^ ]*}} <col:3, col:28> col:28 zero_uwb 'typeof
(0uwb)':'unsigned _BitInt(1)'
+ typedef __typeof__(0uwb) zero_uwb;
+ // CHECK: TypedefDecl 0x{{[^ ]*}} <col:3, col:29> col:29 neg_zero_uwb
'typeof (-0uwb)':'unsigned _BitInt(1)'
----------------
I think by my reading, wbu is valid as well, right? Can we have 1 test for
that?
================
Comment at: clang/test/Lexer/bitint-constants.c:13
+#endif
+#if !(-1uwb > 0)
+#error "uwb suffix must be interpreted as unsigned"
----------------
Ah, oh?! interesting...
================
Comment at: clang/test/Lexer/bitint-constants.c:133
+ // the width of BITINT_MAXWIDTH.
+ _Static_assert(__BITINT_MAXWIDTH__ <= 128,
+ "Need to pick a bigger constant for the test case below.");
----------------
<3
I'd also like a 'TODO' here when this happens that we have something to make
sure that a 129bit/etc literal 'works' as expected.
================
Comment at: llvm/lib/Support/APInt.cpp:505
+unsigned APInt::getSufficientBitsNeeded(StringRef Str, uint8_t Radix) {
+ assert(!Str.empty() && "Invalid string length");
----------------
I hope someone better at bitnonsense (@craig.topper ?) could take a look at
this.
I'm confused why we have to calculate IsNegative 2x, and why 'getBitsNeeded'
needs more work for the 10/36 radix cases, but sufficient does not?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120770/new/
https://reviews.llvm.org/D120770
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
