EricWF marked 10 inline comments as done.
EricWF added inline comments.
================
Comment at: include/clang/AST/Type.h:3990
/// The untransformed type.
- QualType BaseType;
+ SmallVector<QualType, 2> ArgTypes;
----------------
rsmith wrote:
> 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.
Ack. Tail-allocation seems tricky due to `DependentTransformTraitType`. I'll go
ahead with using `ASTContext::Allocate`.
================
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">;
----------------
rsmith wrote:
> Hmm, do you need to touch this diagnostic in this change?
>
> (If so, can you fix the second select to use %plural?)
The changes aren't currently needed. I'll remove them for now.
================
Comment at: include/clang/Serialization/ASTBitCodes.h:1088
/// \brief A DeducedTemplateSpecializationType record.
TYPE_DEDUCED_TEMPLATE_SPECIALIZATION = 45,
----------------
rsmith wrote:
> Please commit the whitespace changes in this file separately.
Darn `git clang-format` being too aggressive.
================
Comment at: lib/AST/ItaniumMangle.cpp:3250-3251
- mangleType(T->getBaseType());
+ for (auto Ty : T->getArgs())
+ mangleType(Ty);
}
----------------
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`.
================
Comment at: lib/AST/TypePrinter.cpp:901
+ return;
+ llvm_unreachable("transformation trait not handled");
}
----------------
rsmith wrote:
> This doesn't look right...
Woops, a funky merge + clang-format strike again!
================
Comment at: lib/Sema/SemaType.cpp:1508-1509
+ break;
+ default:
+ llvm_unreachable("unhandled case");
+ }
----------------
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?
================
Comment at: lib/Sema/SemaType.cpp:8004-8013
+ unsigned MinArity, MaxArity;
+ switch (Kind) {
+ case TransformTraitType::EnumUnderlyingType:
+ MinArity = MaxArity = 1;
+ break;
+ }
+ struct DiagInfo {
----------------
rsmith wrote:
> Looks like you were half-way through factoring out the commonality between
> this and the parser's version?
That was my goal, but I wasn't sure where it was appropriate to put this so it
could be shared across components.
Any suggestions?
https://reviews.llvm.org/D45131
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
