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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: include/clang/AST/Type.h:3990
   /// The untransformed type.
-  QualType BaseType;
+  SmallVector<QualType, 2> ArgTypes;
 
----------------
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.


================
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">;
----------------
Hmm, do you need to touch this diagnostic in this change?

(If so, can you fix the second select to use %plural?)


================
Comment at: include/clang/Basic/Specifiers.h:80
 #include "clang/Basic/OpenCLImageTypes.def"
     TST_error // erroneous type
   };
----------------
Not sure what happened here, these whitespace changes look unrelated to your patch.


================
Comment at: include/clang/Serialization/ASTBitCodes.h:1088
 
       /// \brief A DeducedTemplateSpecializationType record.
       TYPE_DEDUCED_TEMPLATE_SPECIALIZATION = 45,
----------------
Please commit the whitespace changes in this file separately.


================
Comment at: lib/AST/ASTDumper.cpp:2219
+  OS << " " << (Node->isPostfix() ? "postfix" : "prefix") << " '"
+     << UnaryOperator::getOpcodeStr(Node->getOpcode()) << "'";
+  if (!Node->canOverflow())
----------------
I prefer this as it was before (`'`s wrapping the thing they're quoting)...


================
Comment at: lib/AST/ASTImporter.cpp:717
+QualType ASTNodeImporter::VisitTransformTraitType(const TransformTraitType *T) {
+  SmallVector<QualType, 2> ToArgTypes;
+  for (auto Ty : T->getArgs()) {
----------------
2 seems unnecessarily low here.


================
Comment at: lib/AST/ASTStructuralEquivalence.cpp:502
+  case Type::TransformTrait: {
+    const TransformTraitType *TT1 = cast<TransformTraitType>(T1);
+    const TransformTraitType *TT2 = cast<TransformTraitType>(T2);
----------------
This should presumably also check the kind...


================
Comment at: lib/AST/ItaniumMangle.cpp:3250-3251
 
-  mangleType(T->getBaseType());
+  for (auto Ty : T->getArgs())
+    mangleType(Ty);
 }
----------------
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....)


================
Comment at: lib/AST/TypePrinter.cpp:901
+    return;
+  llvm_unreachable("transformation trait not handled");
 }
----------------
This doesn't look right...


================
Comment at: lib/Parse/ParseDeclCXX.cpp:1077-1087
+  auto DiagSelect = [&]() -> Optional<DiagInfo> {
+    if (Info.MinArity && Info.MinArity == Info.MaxArity &&
+        Info.MinArity != Args.size())
+      return DiagInfo{Info.MinArity, 0};
+    if (Info.MinArity && Args.size() < Info.MinArity)
+      return DiagInfo{Info.MinArity, 1};
+    if (Info.MaxArity && Args.size() > Info.MaxArity)
----------------
This only works if none of the arguments are pack expansions.


================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:5367-5368
+      }
+      MarkUsedTemplateParameters(Ctx, TT->getTransformedType(), OnlyDeduced,
+                                 Depth, Used);
+    }
----------------
Marking the transformed type doesn't seem necessary. It can't possibly name template parameters (except via the inputs to the trait).


================
Comment at: lib/Sema/SemaType.cpp:1508-1509
+      break;
+    default:
+      llvm_unreachable("unhandled case");
+    }
----------------
Try to avoid `default:` cases like this, as they suppress the warning notifying the user to update this switch when new traits are added.


================
Comment at: lib/Sema/SemaType.cpp:8004-8013
+  unsigned MinArity, MaxArity;
+  switch (Kind) {
+  case TransformTraitType::EnumUnderlyingType:
+    MinArity = MaxArity = 1;
+    break;
+  }
+  struct DiagInfo {
----------------
Looks like you were half-way through factoring out the commonality between this and the parser's version?


================
Comment at: lib/Sema/SemaType.cpp:8037-8038
                                        SourceLocation Loc) {
-  switch (UKind) {
-  case UnaryTransformType::EnumUnderlyingType:
+  if (diagnoseTransformTraitArity(*this, TKind, Loc, ArgTypes.size()))
+    return QualType();
+  switch (TKind) {
----------------
You'll need to deal with the case that the arguments contain unexpanded packs here before assuming that `ArgTypes.size()` is meaningful.


================
Comment at: lib/Sema/TreeTransform.h:625-627
+  bool TransformTypeSourceListAndExpandPacks(
+      ArrayRef<TypeSourceInfo *> InArgs, bool &ArgChanged,
+      SmallVectorImpl<TypeSourceInfo *> &Args);
----------------
I would drop the "Source" from this. We generally don't shorten "TypeSourceInfo" to "TypeSource", and it's clear enough to just say "Type" here I think. I'd also drop the "AndExpandPacks" because that's just a part of what it means to transform a list.


https://reviews.llvm.org/D45131