[PATCH] Fix incorrect substitution substituting into variadic alias template

[Richard Smith]

On Thu, Jul 18, 2013 at 5:51 PM, Eli Friedman <eli.friedman at gmail.com>wrote:

> New patch attached.  It should be much more clear what it is and is
> not doing.  (Also added the template template parameter test.)
>

LGTM (but we seem to have some remaining problems -- see below)


> 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.
>

Some (but not all) cases where we expand parameter packs do detect the case
where the transformed result contains an unexpanded pack and rebuild a pack
expansion around it. Looking for callers of TryExpandParameterPacks in
TreeTransform, I see:

TransformExprs, TransformTemplateArguments, TransformObjCDictionaryLiteral
get this right.

TransformFunctionTypeParams, TransformTypeTraitExpr, TransformLambdaScope
get this wrong.

TransformSizeOfPackExpr gets this very wrong -- that's PR14858.

>>> 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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130719/b53fee71/attachment.html>