aaron.ballman added inline comments.
================ Comment at: clang/include/clang/AST/Type.h:6199-6200 + friend class ASTContext; + bool IsUnsigned; + unsigned NumBits; + ---------------- Would it be a bad idea to use a bit-field here to smash these fields together? (I don't want to limit the number of bits we can support, but I think it's nice to keep type objects small.) ================ Comment at: clang/include/clang/AST/Type.h:6206 +public: + bool isUnsigned() const { return IsUnsigned; } + unsigned getNumBits() const { return NumBits; } ---------------- We might as well add `isSigned()` as well and convert some of the `!isUnsigned()` calls to use it. ================ Comment at: clang/include/clang/AST/Type.h:6228-6229 + const ASTContext &Context; + bool IsUnsigned; + Expr *NumBitsExpr; + ---------------- Same question here -- we could use a `PointerIntPair`. ================ Comment at: clang/include/clang/AST/Type.h:6236 +public: + bool isUnsigned() const { return IsUnsigned; } + Expr *getNumBitsExpr() const { return NumBitsExpr; } ---------------- Similar here. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10380 +def err_ext_int_bad_size : Error<"%select{signed|unsigned}0 _ExtInt must " + "have a size of at least %select{2|1}0">; +def err_ext_int_max_size : Error<"%select{signed|unsigned}0 _ExtInt of sizes " ---------------- I think we should say "bits" in here to clarify what units the size is measured in. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10382 +def err_ext_int_max_size : Error<"%select{signed|unsigned}0 _ExtInt of sizes " + "greater than %1 not supported">; } // end of sema component. ---------------- Same here. ================ Comment at: clang/lib/AST/ASTContext.cpp:9288-9291 + bool LHSUnsigned = LHS->getAs<ExtIntType>()->isUnsigned(); + bool RHSUnsigned = RHS->getAs<ExtIntType>()->isUnsigned(); + unsigned LHSBits = LHS->getAs<ExtIntType>()->getNumBits(); + unsigned RHSBits = RHS->getAs<ExtIntType>()->getNumBits(); ---------------- `castAs<>` because we already know the expected types? ================ Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3705 + llvm::IntegerType *Ty; + if (llvm::VectorType *VT = dyn_cast<llvm::VectorType>(LHS->getType())) + Ty = cast<llvm::IntegerType>(VT->getElementType()); ---------------- `auto *` ================ Comment at: clang/lib/Sema/SemaDecl.cpp:14558 + // this doesn't use 'isIntegralType' despite the error message mentioning + // integral type because isIntegralType would also allow enum types in C. ---------------- this -> This ================ Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2113 + + if (const auto *IntArg = cast<ExtIntType>(Arg)){ + if (IntParam->isUnsigned() != IntArg->isUnsigned()) ---------------- Did you mean to use `dyn_cast<>` here instead? ================ Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2130 + + if (const auto *IntArg = cast<DependentExtIntType>(Arg)) { + if (IntParam->isUnsigned() != IntArg->isUnsigned()) ---------------- Did you mean to use `dyn_cast<>` here instead? ================ Comment at: clang/lib/Sema/SemaType.cpp:2168 +/// +/// \param Sign TypeSpecifierSign of this type. +/// ---------------- Comment doesn't match the code. ================ Comment at: clang/lib/Sema/SemaType.cpp:2197-2198 + if (NumBits > llvm::IntegerType::MAX_INT_BITS) { + Diag(Loc, diag::err_ext_int_max_size) << IsUnsigned + << llvm::IntegerType::MAX_INT_BITS; + return QualType(); ---------------- Formatting looks slightly off here (the indentation for the second trailing argument looks funky, but maybe that's just Phab). ================ Comment at: clang/test/CodeGen/ext-int.cpp:6 +// RUN: %clang_cc1 -triple x86_64-windows-pc -O3 -disable-llvm-passes -new-struct-path-tbaa -emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,WIN,NewStructPathTBAA + + ---------------- I'd appreciate some codegen tests that show how this type works with variadic functions in C and demonstrate that you can pass these values and pull them back out via `va_arg`. ================ Comment at: clang/test/SemaCXX/ext-int.cpp:103 + _ExtInt(43) x43_s = 1, y43_s = 1; + _ExtInt(sizeof(int) * 8) x32_s = 1, y32_s = 1; + unsigned _ExtInt(sizeof(unsigned) * 8) x32_u = 1, y32_u = 1; ---------------- I'd also like a test like: ``` constexpr int func() { return 42; } _ExtInt(func()) ThisShouldWorkRight; ``` ================ Comment at: clang/test/SemaCXX/ext-int.cpp:206-207 + + static_assert(sizeof(_ExtInt(3340)) == 424, ""); // 424 * 8 == 3392. + static_assert(sizeof(_ExtInt(1049)) == 136, ""); // 136 * 8 == 1088. + ---------------- I'd like to see some similar tests for `alignof` ================ Comment at: clang/test/SemaCXX/ext-int.cpp:252 + +// Note, the extra closing brackets confuses the parser, so this test should likely be last. +// expected-error@+5{{expected ')'}} ---------------- These tests should be in a parsing test rather than in sema. ================ Comment at: clang/test/SemaCXX/ext-int.cpp:266 +_ExtInt{32} c; + ---------------- Some additional sema/codegen tests I'd like to see: * That this new type works with typeid in C++ (that two types with the same sign and width match, and distinct types don't) * The behavior of explicit casts to this type and from this type in both C and C++ * C test that this behaves properly in a _Generic selection expression with other _ExtInt bit-width and sign types * C test that we can properly create VLAs of _ExtInt type * C and C++ tests that `offsetof` behaves properly on structures with these types CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73967/new/ https://reviews.llvm.org/D73967 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits