[PATCH] D73967: Implement _ExtInt as an extended int type specifier.

[Aaron Ballman via Phabricator via cfe-commits]

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 at +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