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

[Eric Fiselier via Phabricator via cfe-commits]

EricWF planned changes to this revision.
EricWF marked 7 inline comments as done.
EricWF added inline comments.


================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:5262-5266
+AST_MATCHER(Type, unaryTransformType) {
+  if (const auto *T = Node.getAs<TransformTraitType>())
+    return T->getNumArgs() == 1;
+  return false;
+}
----------------
rsmith wrote:
> Generally I think we want the primitive AST matchers to pretty directly correspond to the Clang AST. I think it would make sense to replace this matcher with a generalized `transformTrait` matcher -- but in a separate commit from this one. This is OK for now.
I added a generalized `transformTrait` matcher, but forgot to declare it.

The reason for keeping the `unaryTransformType` was simply to avoid breaking existing code which already depends on it.
My plan was to possibly remove this wrapper later, after cleaning up usages in the LLVM code base.


================
Comment at: lib/AST/Type.cpp:3089
+      this->setContainsUnexpandedParameterPack(true);
+  }
+}
----------------
rsmith wrote:
> (And just for clarity: instantiation-dependence and contains-unexpanded-parameter-pack are concerned with the syntactic form of the type, not the semantics, so those two *should* be based on the corresponding properties of the arguments.)
Thanks for the clarification. I was about to ask.


================
Comment at: lib/Sema/SemaType.cpp:1508-1509
+      break;
+    default:
+      llvm_unreachable("unhandled case");
+    }
----------------
rsmith wrote:
> EricWF wrote:
> > 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?
> I think you should convert the trait to a `QualType` when you parse it, rather than waiting until now, which would make this moot.
Hmm. I'm not quite sure how/where to do that in the parser.

Are you suggesting calling `BuildTransformTrait` from inside `ParseTransformTraitTypeSpecifier`, and instead of building the `DeclSpec` containing the result of `BuildTransformTrait` instead of the list of argument type?


================
Comment at: lib/Sema/SemaType.cpp:8014-8022
+  if (DiagSelect.hasValue()) {
+    auto Info = DiagSelect.getValue();
+    PartialDiagnostic PD = PDiag(diag::err_type_trait_arity);
+    PD << Info.ReqNumArgs << Info.SelectOne << (Info.ReqNumArgs != 1)
+       << (int)NumArgs << R;
+    return PD;
+  }
----------------
rsmith wrote:
> Do you really need a `PartialDiagnostic` here? (Could you directly emit the diagnostic instead?)
As it stands now, I believe I do, since the diagnostic needs to be emitted in both the parser and sema. If I'm not mistaken it would be incorrect to use Sema's diagnostic engine inside the parser and vice-versa.

However, if I delay the arity check until we're in `SemaType` as you suggested elsewhere, then the partial diagnostic is unneeded.


================
Comment at: lib/Sema/SemaType.cpp:8036
 
-        DiagnoseUseOfDecl(ED, Loc);
+  // Delay all checking while any of the arguments are instantiation dependent.
+  if (IsInstantDependent)
----------------
rsmith wrote:
> This doesn't seem right; you should delay if any of the arguments is dependent, but instantiation-dependent / containing a pack don't seem like they should matter here.
We can't check the arity here as long as any of the arguments contain unexpanded parameter packs, so at least in that case we need to delay.

However, I'll change the `isInstantationDependent` check to `isDependent`.


https://reviews.llvm.org/D45131