[PATCH] D45131: [AST] Refactor UnaryTransformType into TransformTraitType supporting non-unary transforms

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 10 inline comments as done.
EricWF added inline comments.


================
Comment at: include/clang/AST/Type.h:3990
   /// The untransformed type.
-  QualType BaseType;
+  SmallVector<QualType, 2> ArgTypes;
 
----------------
rsmith wrote:
> You can't store types with non-trivial destructors on AST nodes; the Type destructor will never run, so you'll leak. Instead, consider tail-allocating this list of QualTypes, or performing a separate allocation using the ASTContext slab allocator to hold the list if tail-allocation isn't feasible for some reason.
Ack. Tail-allocation seems tricky due to `DependentTransformTraitType`. I'll go ahead with using `ASTContext::Allocate`.


================
Comment at: include/clang/Basic/DiagnosticCommonKinds.td:111-114
+// Type traits
+def err_type_trait_arity : Error<
+  "type trait requires %0%select{| or more| or fewer}1 argument%select{|s}2; have "
+  "%3 argument%s3">;
----------------
rsmith wrote:
> Hmm, do you need to touch this diagnostic in this change?
> 
> (If so, can you fix the second select to use %plural?)
The changes aren't currently needed. I'll remove them for now.


================
Comment at: include/clang/Serialization/ASTBitCodes.h:1088
 
       /// \brief A DeducedTemplateSpecializationType record.
       TYPE_DEDUCED_TEMPLATE_SPECIALIZATION = 45,
----------------
rsmith wrote:
> Please commit the whitespace changes in this file separately.
Darn `git clang-format` being too aggressive.


================
Comment at: lib/AST/ItaniumMangle.cpp:3250-3251
 
-  mangleType(T->getBaseType());
+  for (auto Ty : T->getArgs())
+    mangleType(Ty);
 }
----------------
rsmith wrote:
> We need manglings to be self-delimiting, and we can't tell where the end of the argument list is with this mangling approach. :(
> 
> (Eg, `f(__raw_invocation_type(T1, T2, T3))` and `f(__raw_invocation_type(T1, T2), T3)` would mangle the same with this mangling.)
> 
> Maybe drop an `E` on the end? (Or maybe do that only for traits that don't require exactly one argument, but that would create pain for demanglers....)
Makes sense. Thanks for the explanation. I'll go ahead and drop and E on the end.

However, will this cause ABI issues? Either with existing code or with GCC? If so, perhaps we should maintain the current mangling for `__underlying_type`.


================
Comment at: lib/AST/TypePrinter.cpp:901
+    return;
+  llvm_unreachable("transformation trait not handled");
 }
----------------
rsmith wrote:
> This doesn't look right...
Woops, a funky merge + clang-format strike again!


================
Comment at: lib/Sema/SemaType.cpp:1508-1509
+      break;
+    default:
+      llvm_unreachable("unhandled case");
+    }
----------------
rsmith wrote:
> Try to avoid `default:` cases like this, as they suppress the warning notifying the user to update this switch when new traits are added.
Understood, but surely this is a case where `default:` is warranted.  We're switching over a range of values much larger than `TransformTraitType::TTKind` in order to transform it into the `TTKind`

Do you have suggestions for improving it?


================
Comment at: lib/Sema/SemaType.cpp:8004-8013
+  unsigned MinArity, MaxArity;
+  switch (Kind) {
+  case TransformTraitType::EnumUnderlyingType:
+    MinArity = MaxArity = 1;
+    break;
+  }
+  struct DiagInfo {
----------------
rsmith wrote:
> Looks like you were half-way through factoring out the commonality between this and the parser's version?
That was my goal, but I wasn't sure where it was appropriate to put this so it could be shared across components. 

Any suggestions?


https://reviews.llvm.org/D45131