[PATCH] D45131: [AST] Refactor UnaryTransformType into TransformTraitType supporting non-unary transforms
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu May 3 15:44:51 PDT 2018
rsmith added inline comments.
================
Comment at: include/clang/AST/TypeLoc.h:1992-1993
+ unsigned getExtraLocalDataAlignment() const {
+ static_assert(alignof(TransformTraitTypeLoc) >= alignof(TypeSourceInfo *),
+ "not enough alignment for tail-allocated data");
+ return alignof(TypeSourceInfo *);
----------------
This assert doesn't make sense: the extra `TypeSourceInfo*`s are not stored after a `TransformTraitTypeLoc`. Rather, `*TypeLoc` is a non-owning veneer around separately-allocated data. `ConcreteTypeLoc` takes care of adding any necessary padding to get to the right alignment for you.
================
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;
+}
----------------
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.
================
Comment at: lib/AST/ASTContext.cpp:4630
-/// getUnaryTransformationType - We don't unique these, since the memory
+/// getTransformTraitType - We don't unique these, since the memory
/// savings are minimal and these are rare.
----------------
You can just drop the function name (and hyphen) here; that's an obsolescent style.
================
Comment at: lib/AST/ItaniumMangle.cpp:3250-3251
- mangleType(T->getBaseType());
+ for (auto Ty : T->getArgs())
+ mangleType(Ty);
}
----------------
EricWF wrote:
> EricWF wrote:
> > 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`.
> To answer my own question, GCC doesn't mangle `__underlying_type` yet. And I doubt that people are currently depending on the dependent mangling of `__underlying_type`. Perhaps I'm wrong about that last assumption though.
The existing `U3eut` mangling (with no terminating `E`) was approximately OK, following http://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangle.qualified-type. It would be better to use `U17__underlying_type`, though. This is not exactly right, since it treats `__underlying_type` as a type qualifier rather than a type function, but that's not too far off.
The new mangling doesn't match the Itanium ABI rule for vendor extensions. We basically have a choice between `u` <source-name> (which doesn't give us a good way to encode the arguments) or `U` <source-name> <template-args> <type> (which could work, but we'd need to make a somewhat-arbitrary choice of which type we consider to be the "main" type to which the qualifier applies).
We could arbitrarily say:
* the first type in the trait is the "main" type
* the rest of the trait is mangled as a qualifier
* the qualifier is given template-args if and only if there is more than one argument
So:
* `__underlying_type(T)` would mangle as `U17__underlying_type1T` (or approximately `T __underlying_type`)
* `__raw_invocation_type(F, A1, A2)` would mangle as `U21__raw_invocation_typeI2A12A2E1F` (or approximately `F __raw_invocation_type<A1, A2>`)
* `__raw_invocation_type(F)` would mangle as `U21__raw_invocation_type1F` (or approximately `F __raw_invocation_type`)
Or we could track here whether the trait can accept >1 argument, and always use the template-args formulation if so. I have no strong opinions either way.
Or we could suggest an Itanium ABI extension to permit <template-args> for the `u...` vendor extension type form, for vendor type traits. That'd lead to:
* `__underlying_type(T)` would mangle as `u17__underlying_typeI1TE` (or approximately `__underlying_type<T>`)
* `__raw_invocation_type(F, A1, A2)` would mangle as `u21__raw_invocation_typeI1F2A12A2E` (or approximately `__raw_invocation_type<F, A1, A2>`)
* `__raw_invocation_type(F)` would mangle as `u21__raw_invocation_typeI1FE` (or approximately `__raw_invocation_type<F>`)
... and we could encourage demanglers to use parens instead of angles for those arguments.
The final of the above options seems best to me. What do you think?
================
Comment at: lib/AST/Type.cpp:3081-3082
+ new ((void *)(ArgStorage + I)) QualType(T);
+ if (T->isDependentType())
+ this->setDependent(true);
+ if (T->isInstantiationDependentType())
----------------
This should come from the transformed type, not the base type. (A transform trait could turn a dependent type into a non-depnedent type.)
================
Comment at: lib/AST/Type.cpp:3085-3086
+ this->setInstantiationDependent(true);
+ if (T->isVariablyModifiedType())
+ this->setVariablyModified(true);
+ if (T->containsUnexpandedParameterPack())
----------------
Likewise, there's no reason to think that variably-modifiedness of the arguments has anything to do with variably-modifiedness of the transformed type. (Eg, `__raw_invocation_type(Fn, VLAType)` is not going to be a VLA type.)
================
Comment at: lib/AST/Type.cpp:3089
+ this->setContainsUnexpandedParameterPack(true);
+ }
+}
----------------
(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.)
================
Comment at: lib/Lex/PPMacroExpansion.cpp:1245
//.Case("cxx_concepts", LangOpts.CPlusPlusTSConcepts)
// FIXME: Should this be __has_feature or __has_extension?
// Type traits
----------------
Remove this fixme too, please. (But let's hold off on this change until you land the `__raw_invocation_type` patch to keep this patch more focused.)
================
Comment at: lib/Parse/ParseDeclCXX.cpp:1058
+ if (Ty.isInvalid()) {
+ Parens.skipToEnd();
+ return;
----------------
Call `DS.SetTypeSpecError()` here and at other points where you bail out of this function.
================
Comment at: lib/Parse/ParseDeclCXX.cpp:1073-1074
+ if (!HasPackExpand) {
+ auto PDiag = Actions.CheckTransformTraitArity(KindInfo.first, Args.size(),
+ SourceRange(StartLoc));
+ if (PDiag.hasValue()) {
----------------
Instead of performing an ad-hoc arity check here, perform the check in the code that converts a kind + list of arguments into a `TransformTraitType`. That way there's no way to forget these checks and no side channel state to track about whether you've already done them.
================
Comment at: lib/Sema/DeclSpec.cpp:369-370
case TST_underlyingType:
+ return false;
+
----------------
This is going to be problematic down the line -- I expect there are going to be type transform traits that can produce function types. I think this strongly argues that we need to have already converted the trait into a `QualType` by the time we get here, rather than representing it as a trait kind and a list of types in the `DeclSpec` and deferring the conversion to later.
================
Comment at: lib/Sema/SemaType.cpp:1508-1509
+ break;
+ default:
+ llvm_unreachable("unhandled case");
+ }
----------------
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.
================
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;
+ }
----------------
Do you really need a `PartialDiagnostic` here? (Could you directly emit the diagnostic instead?)
================
Comment at: lib/Sema/SemaType.cpp:8027-8029
+ auto MakeTrait = [&](QualType TransformedType) {
+ return Context.getTransformTraitType(ArgTypes, TransformedType, TKind);
+ };
----------------
I think this would be clearer if split into a function that computes and returns the result type of the trait and code to build the trait from that type, rather than this factored-out lambda approach -- this use of a lambda hides the fundamental behavior of this function, which is "first figure out the type and then build a trait producing that type".
================
Comment at: lib/Sema/SemaType.cpp:8036
- DiagnoseUseOfDecl(ED, Loc);
+ // Delay all checking while any of the arguments are instantiation dependent.
+ if (IsInstantDependent)
----------------
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.
https://reviews.llvm.org/D45131
More information about the cfe-commits
mailing list