aaron.ballman 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)
----------------
erichkeane wrote:
> 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?
That seems plausible, but perhaps as a follow-up. We're going from 12 bits to
13 with this patch (and correctly a layout issue as a drive-by), so we're not
at or near an allocation unit boundary.
================
Comment at: clang/lib/AST/StmtPrinter.cpp:1157
+ if (const auto *BT = Node->getType()->getAs<BitIntType>()) {
+ OS << (BT->isUnsigned() ? "uwb" : "wb");
+ return;
----------------
erichkeane wrote:
> Nit: Instead of BT->isUnsigned(), we already have isSigned above.
Good call!
================
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.
----------------
erichkeane wrote:
> Do we also have to reject isImaginary/isFract/etc separately?
I'm not super familiar with fixed-point literals, but at least with imaginary,
you can make a _Complex from any integer type, including _BitInt, so I don't
think there's a reason to reject that case.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:3987
+ << Literal.isUnsigned;
+ Width = MaxBitIntWidth;
+ }
----------------
erichkeane wrote:
> Do you have some AST test here to see what happens to the value when the
> width is exceeded?
Not an AST test -- can't get one of those because this emits an error. But we
do have a semantic test, see bitint-constants.c lines 135-136.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:3995
+ ResultVal = ResultVal.zextOrTrunc(Width);
+ Ty = Context.getBitIntType(Literal.isUnsigned, Width);
+ }
----------------
erichkeane wrote:
> I think it is already covered, but do we make sure the literal itself and the
> suffix match? That is: -111uwb is invalid?
We do not, but that's consistent with other suffix confusion:
https://godbolt.org/z/rExPGdex3
(Remember, the `-` isn't part of the literal, that's a unary negation operator
applied to the positive literal.)
================
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)'
----------------
erichkeane wrote:
> I think by my reading, wbu is valid as well, right? Can we have 1 test for
> that?
We have one -- bitint-constant.c lines 89-92.
================
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.");
----------------
erichkeane wrote:
> <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.
Added a comment.
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
