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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
