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

[Eric Fiselier via Phabricator via cfe-commits]

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


================
Comment at: lib/AST/ItaniumMangle.cpp:3250-3251
 
-  mangleType(T->getBaseType());
+  for (auto Ty : T->getArgs())
+    mangleType(Ty);
 }
----------------
rsmith wrote:
> 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?
> Or we could suggest an Itanium ABI extension to permit <template-args> for the u... vendor extension type form,
> [...]
> The final of the above options seems best to me. What do you think?

I think the final form would also be preferable. I'll go ahead and implement it.
What would it involve to suggest the Itanium ABI extension?



================
Comment at: lib/Sema/DeclSpec.cpp:369-370
      
     case TST_underlyingType:
+      return false;
+
----------------
rsmith wrote:
> 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.
Ack. Done.


================
Comment at: lib/Sema/SemaType.cpp:1508-1509
+      break;
+    default:
+      llvm_unreachable("unhandled case");
+    }
----------------
EricWF wrote:
> 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?
OK, I've got it all figured out. The requested change has been made.


https://reviews.llvm.org/D45131