[PATCH] D120770: [C2x] Implement literal suffixes for _BitInt

[Erich Keane via Phabricator via llvm-commits]

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