[PATCH] Fix incorrect substitution substituting into variadic alias template
Eli Friedman
eli.friedman at gmail.com
Thu Jul 18 17:51:18 PDT 2013
New patch attached. It should be much more clear what it is and is
not doing. (Also added the template template parameter test.)
-Eli
On Wed, Jul 17, 2013 at 6:58 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> On Wed, Jul 17, 2013 at 6:01 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> On Wed, Jul 17, 2013 at 5:43 PM, Eli Friedman <eli.friedman at gmail.com>
>> wrote:
>>>
>>> Patch attached. Fixes PR16646. The concept is that when substitution
>>> replaces a pack with another pack, we don't want to expand the pack
>>> immediately: we want to expand it at the same level as the original
>>> pack.
>>
>>
>> How do we ensure that the we retain a pack expansion around the transformed
>> entity?
>
> I'm pretty sure that if we find a template argument which is a pack
> expansion, the input TemplateTypeParmType has to be an unexpanded
> parameter pack; if I'm wrong about that, the code should be moved a
> bit. I'll probably end up moving it anyway to make it more clear.
>
> If we are expanding a parameter pack, we must be in the context of an
> ArgumentPackSubstitutionIndexRAII which should expect the possibility
> of an unexpanded parameter pack.
>
>>> I'm not confident that this patch is correct, though: just stripping
>>> off the PackExpansionType in the middle of instantiation seems weird.
>>> Also, I'm not sure what other tests would be relevant.
>>
>>
>> The patch only covers type template parameters; presumably, the same issue
>> applies to non-type template parameters and template template parameters?
>
> We have code which I'm pretty sure is equivalent for non-type template
> parameters; see the beginning of
> TemplateInstantiator::transformNonTypeTemplateParmRef. The following
> testcase works without any other modifications:
>
> template <int x> struct DefaultValue { const int value = x;};
> template <typename ... Args> struct tuple {};
> template <int ... Args> using Zero = tuple<DefaultValue<Args> ...>;
> template <int ... Args> void f(const Zero<Args ...> &t);
> void f() {
> f(Zero<1,2,3>());
> }
>
> On the other hand, the following currently crashes:
>
> template<int x> struct X {};
> template <template<int x> class temp> struct DefaultValue { const
> temp<0> value; };
> template <typename ... Args> struct tuple {};
> template <template<int x> class... Args> using Zero =
> tuple<DefaultValue<Args> ...>;
> template <template<int x> class... Args> void f(const Zero<Args ...> &t);
> void f() {
> f(Zero<X,X,X>());
> }
>
> Conceptually, it's the same sort of fix, though.
>
> I should probably use TemplateArgument::getPackExpansionPattern
> consistently, instead of directly checking for a PackExpansionType.
>
>> Does this correctly handle expanding the pack into a fixed set of parameters
>> and a pack, in cases like:
>>
>> template<typename ...Ts> struct S {};
>> template<typename ...Ts> using X = S<S<Ts>...>;
>> template<typename ...Ts> void f(X<int, Ts...> x);
>> void g() { f(X<int, int, int>()); }
>>
>> ? (I think it should, but this seems worth testing.)
>
> It does.
>
> Thanks for asking the right questions. I learned a lot more about my
> patch by writing up the answers. :)
-------------- next part --------------
diff --git a/lib/Sema/SemaTemplateInstantiate.cpp b/lib/Sema/SemaTemplateInstantiate.cpp
index e86c742..3904daa 100644
--- a/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/lib/Sema/SemaTemplateInstantiate.cpp
@@ -883,6 +883,16 @@ bool TemplateInstantiator::AlreadyTransformed(QualType T) {
return true;
}
+static TemplateArgument
+getPackSubstitutedTemplateArgument(Sema &S, TemplateArgument Arg) {
+ assert(S.ArgumentPackSubstitutionIndex >= 0);
+ assert(S.ArgumentPackSubstitutionIndex < (int)Arg.pack_size());
+ Arg = Arg.pack_begin()[S.ArgumentPackSubstitutionIndex];
+ if (Arg.isPackExpansion())
+ Arg = Arg.getPackExpansionPattern();
+ return Arg;
+}
+
Decl *TemplateInstantiator::TransformDecl(SourceLocation Loc, Decl *D) {
if (!D)
return 0;
@@ -902,10 +912,7 @@ Decl *TemplateInstantiator::TransformDecl(SourceLocation Loc, Decl *D) {
if (TTP->isParameterPack()) {
assert(Arg.getKind() == TemplateArgument::Pack &&
"Missing argument pack");
-
- assert(getSema().ArgumentPackSubstitutionIndex >= 0);
- assert(getSema().ArgumentPackSubstitutionIndex < (int)Arg.pack_size());
- Arg = Arg.pack_begin()[getSema().ArgumentPackSubstitutionIndex];
+ Arg = getPackSubstitutedTemplateArgument(getSema(), Arg);
}
TemplateName Template = Arg.getAsTemplate();
@@ -950,8 +957,7 @@ TemplateInstantiator::TransformFirstQualifierInScope(NamedDecl *D,
if (getSema().ArgumentPackSubstitutionIndex == -1)
return 0;
- assert(getSema().ArgumentPackSubstitutionIndex < (int)Arg.pack_size());
- Arg = Arg.pack_begin()[getSema().ArgumentPackSubstitutionIndex];
+ Arg = getPackSubstitutedTemplateArgument(getSema(), Arg);
}
QualType T = Arg.getAsType();
@@ -1053,9 +1059,8 @@ TemplateName TemplateInstantiator::TransformTemplateName(CXXScopeSpec &SS,
// keep the entire argument pack.
return getSema().Context.getSubstTemplateTemplateParmPack(TTP, Arg);
}
-
- assert(getSema().ArgumentPackSubstitutionIndex < (int)Arg.pack_size());
- Arg = Arg.pack_begin()[getSema().ArgumentPackSubstitutionIndex];
+
+ Arg = getPackSubstitutedTemplateArgument(getSema(), Arg);
}
TemplateName Template = Arg.getAsTemplate();
@@ -1077,11 +1082,9 @@ TemplateName TemplateInstantiator::TransformTemplateName(CXXScopeSpec &SS,
if (getSema().ArgumentPackSubstitutionIndex == -1)
return Name;
- const TemplateArgument &ArgPack = SubstPack->getArgumentPack();
- assert(getSema().ArgumentPackSubstitutionIndex < (int)ArgPack.pack_size() &&
- "Pack substitution index out-of-range");
- return ArgPack.pack_begin()[getSema().ArgumentPackSubstitutionIndex]
- .getAsTemplate();
+ TemplateArgument Arg = SubstPack->getArgumentPack();
+ Arg = getPackSubstitutedTemplateArgument(getSema(), Arg);
+ return Arg.getAsTemplate();
}
return inherited::TransformTemplateName(SS, Name, NameLoc, ObjectType,
@@ -1146,8 +1149,7 @@ TemplateInstantiator::TransformTemplateParmRefExpr(DeclRefExpr *E,
Arg);
}
- assert(getSema().ArgumentPackSubstitutionIndex < (int)Arg.pack_size());
- Arg = Arg.pack_begin()[getSema().ArgumentPackSubstitutionIndex];
+ Arg = getPackSubstitutedTemplateArgument(getSema(), Arg);
}
return transformNonTypeTemplateParmRef(NTTP, E->getLocation(), Arg);
@@ -1160,14 +1162,6 @@ ExprResult TemplateInstantiator::transformNonTypeTemplateParmRef(
ExprResult result;
QualType type;
- // If the argument is a pack expansion, the parameter must actually be a
- // parameter pack, and we should substitute the pattern itself, producing
- // an expression which contains an unexpanded parameter pack.
- if (arg.isPackExpansion()) {
- assert(parm->isParameterPack() && "pack expansion for non-pack");
- arg = arg.getPackExpansionPattern();
- }
-
// The template argument itself might be an expression, in which
// case we just return that expression.
if (arg.getKind() == TemplateArgument::Expression) {
@@ -1233,12 +1227,9 @@ TemplateInstantiator::TransformSubstNonTypeTemplateParmPackExpr(
// We aren't expanding the parameter pack, so just return ourselves.
return getSema().Owned(E);
}
-
- const TemplateArgument &ArgPack = E->getArgumentPack();
- unsigned Index = (unsigned)getSema().ArgumentPackSubstitutionIndex;
- assert(Index < ArgPack.pack_size() && "Substitution index out-of-range");
-
- const TemplateArgument &Arg = ArgPack.pack_begin()[Index];
+
+ TemplateArgument Arg = E->getArgumentPack();
+ Arg = getPackSubstitutedTemplateArgument(getSema(), Arg);
return transformNonTypeTemplateParmRef(E->getParameterPack(),
E->getParameterPackLocation(),
Arg);
@@ -1409,8 +1400,7 @@ TemplateInstantiator::TransformTemplateTypeParmType(TypeLocBuilder &TLB,
return Result;
}
- assert(getSema().ArgumentPackSubstitutionIndex < (int)Arg.pack_size());
- Arg = Arg.pack_begin()[getSema().ArgumentPackSubstitutionIndex];
+ Arg = getPackSubstitutedTemplateArgument(getSema(), Arg);
}
assert(Arg.getKind() == TemplateArgument::Type &&
@@ -1458,12 +1448,11 @@ TemplateInstantiator::TransformSubstTemplateTypeParmPackType(
NewTL.setNameLoc(TL.getNameLoc());
return TL.getType();
}
-
- const TemplateArgument &ArgPack = TL.getTypePtr()->getArgumentPack();
- unsigned Index = (unsigned)getSema().ArgumentPackSubstitutionIndex;
- assert(Index < ArgPack.pack_size() && "Substitution index out-of-range");
-
- QualType Result = ArgPack.pack_begin()[Index].getAsType();
+
+ TemplateArgument Arg = TL.getTypePtr()->getArgumentPack();
+ Arg = getPackSubstitutedTemplateArgument(getSema(), Arg);
+ QualType Result = Arg.getAsType();
+
Result = getSema().Context.getSubstTemplateTypeParmType(
TL.getTypePtr()->getReplacedParameter(),
Result);
diff --git a/test/SemaTemplate/alias-templates.cpp b/test/SemaTemplate/alias-templates.cpp
index 20ba6e0..eeb6b95 100644
--- a/test/SemaTemplate/alias-templates.cpp
+++ b/test/SemaTemplate/alias-templates.cpp
@@ -166,3 +166,26 @@ namespace PR13136 {
return 0;
}
}
+
+namespace PR16646 {
+ namespace test1 {
+ template <typename T> struct DefaultValue { const T value=0;};
+ template <typename ... Args> struct tuple {};
+ template <typename ... Args> using Zero = tuple<DefaultValue<Args> ...>;
+ template <typename ... Args> void f(const Zero<Args ...> &t);
+ void f() {
+ f(Zero<int,double,double>());
+ }
+ }
+
+ namespace test2 {
+ template<int x> struct X {};
+ template <template<int x> class temp> struct DefaultValue { const temp<0> value; };
+ template <typename ... Args> struct tuple {};
+ template <template<int x> class... Args> using Zero = tuple<DefaultValue<Args> ...>;
+ template <template<int x> class... Args> void f(const Zero<Args ...> &t);
+ void f() {
+ f(Zero<X,X,X>());
+ }
+ }
+}
More information about the cfe-commits
mailing list