rsmith added inline comments.
================
Comment at: include/clang/AST/Type.h:3990
/// The untransformed type.
- QualType BaseType;
+ SmallVector<QualType, 2> ArgTypes;
----------------
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.
================
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">;
----------------
Hmm, do you need to touch this diagnostic in this change?
(If so, can you fix the second select to use %plural?)
================
Comment at: include/clang/Basic/Specifiers.h:80
#include "clang/Basic/OpenCLImageTypes.def"
TST_error // erroneous type
};
----------------
Not sure what happened here, these whitespace changes look unrelated to your
patch.
================
Comment at: include/clang/Serialization/ASTBitCodes.h:1088
/// \brief A DeducedTemplateSpecializationType record.
TYPE_DEDUCED_TEMPLATE_SPECIALIZATION = 45,
----------------
Please commit the whitespace changes in this file separately.
================
Comment at: lib/AST/ASTDumper.cpp:2219
+ OS << " " << (Node->isPostfix() ? "postfix" : "prefix") << " '"
+ << UnaryOperator::getOpcodeStr(Node->getOpcode()) << "'";
+ if (!Node->canOverflow())
----------------
I prefer this as it was before (`'`s wrapping the thing they're quoting)...
================
Comment at: lib/AST/ASTImporter.cpp:717
+QualType ASTNodeImporter::VisitTransformTraitType(const TransformTraitType *T)
{
+ SmallVector<QualType, 2> ToArgTypes;
+ for (auto Ty : T->getArgs()) {
----------------
2 seems unnecessarily low here.
================
Comment at: lib/AST/ASTStructuralEquivalence.cpp:502
+ case Type::TransformTrait: {
+ const TransformTraitType *TT1 = cast<TransformTraitType>(T1);
+ const TransformTraitType *TT2 = cast<TransformTraitType>(T2);
----------------
This should presumably also check the kind...
================
Comment at: lib/AST/ItaniumMangle.cpp:3250-3251
- mangleType(T->getBaseType());
+ for (auto Ty : T->getArgs())
+ mangleType(Ty);
}
----------------
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....)
================
Comment at: lib/AST/TypePrinter.cpp:901
+ return;
+ llvm_unreachable("transformation trait not handled");
}
----------------
This doesn't look right...
================
Comment at: lib/Parse/ParseDeclCXX.cpp:1077-1087
+ auto DiagSelect = [&]() -> Optional<DiagInfo> {
+ if (Info.MinArity && Info.MinArity == Info.MaxArity &&
+ Info.MinArity != Args.size())
+ return DiagInfo{Info.MinArity, 0};
+ if (Info.MinArity && Args.size() < Info.MinArity)
+ return DiagInfo{Info.MinArity, 1};
+ if (Info.MaxArity && Args.size() > Info.MaxArity)
----------------
This only works if none of the arguments are pack expansions.
================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:5367-5368
+ }
+ MarkUsedTemplateParameters(Ctx, TT->getTransformedType(), OnlyDeduced,
+ Depth, Used);
+ }
----------------
Marking the transformed type doesn't seem necessary. It can't possibly name
template parameters (except via the inputs to the trait).
================
Comment at: lib/Sema/SemaType.cpp:1508-1509
+ break;
+ default:
+ llvm_unreachable("unhandled case");
+ }
----------------
Try to avoid `default:` cases like this, as they suppress the warning notifying
the user to update this switch when new traits are added.
================
Comment at: lib/Sema/SemaType.cpp:8004-8013
+ unsigned MinArity, MaxArity;
+ switch (Kind) {
+ case TransformTraitType::EnumUnderlyingType:
+ MinArity = MaxArity = 1;
+ break;
+ }
+ struct DiagInfo {
----------------
Looks like you were half-way through factoring out the commonality between this
and the parser's version?
================
Comment at: lib/Sema/SemaType.cpp:8037-8038
SourceLocation Loc) {
- switch (UKind) {
- case UnaryTransformType::EnumUnderlyingType:
+ if (diagnoseTransformTraitArity(*this, TKind, Loc, ArgTypes.size()))
+ return QualType();
+ switch (TKind) {
----------------
You'll need to deal with the case that the arguments contain unexpanded packs
here before assuming that `ArgTypes.size()` is meaningful.
================
Comment at: lib/Sema/TreeTransform.h:625-627
+ bool TransformTypeSourceListAndExpandPacks(
+ ArrayRef<TypeSourceInfo *> InArgs, bool &ArgChanged,
+ SmallVectorImpl<TypeSourceInfo *> &Args);
----------------
I would drop the "Source" from this. We generally don't shorten
"TypeSourceInfo" to "TypeSource", and it's clear enough to just say "Type" here
I think. I'd also drop the "AndExpandPacks" because that's just a part of what
it means to transform a list.
https://reviews.llvm.org/D45131
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
