On Thu, Jul 18, 2013 at 5:51 PM, Eli Friedman <span dir="ltr"><<a href="mailto:eli.friedman@gmail.com" target="_blank">eli.friedman@gmail.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
New patch attached.  It should be much more clear what it is and is<br>
not doing.  (Also added the template template parameter test.)<div class="HOEnZb"><div class="h5"></div></div></blockquote><div><br></div><div>LGTM (but we seem to have some remaining problems -- see below)</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
On Wed, Jul 17, 2013 at 6:58 PM, Eli Friedman <<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>> wrote:<br>
> On Wed, Jul 17, 2013 at 6:01 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
>> On Wed, Jul 17, 2013 at 5:43 PM, Eli Friedman <<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>><br>
>> wrote:<br>
>>><br>
>>> Patch attached.  Fixes PR16646.  The concept is that when substitution<br>
>>> replaces a pack with another pack, we don't want to expand the pack<br>
>>> immediately: we want to expand it at the same level as the original<br>
>>> pack.<br>
>><br>
>><br>
>> How do we ensure that the we retain a pack expansion around the transformed<br>
>> entity?<br>
><br>
> I'm pretty sure that if we find a template argument which is a pack<br>
> expansion, the input TemplateTypeParmType has to be an unexpanded<br>
> parameter pack; if I'm wrong about that, the code should be moved a<br>
> bit.  I'll probably end up moving it anyway to make it more clear.<br>
><br>
> If we are expanding a parameter pack, we must be in the context of an<br>
> ArgumentPackSubstitutionIndexRAII which should expect the possibility<br>
> of an unexpanded parameter pack.<br></div></div></blockquote><div><br></div><div>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:</div>
<div><br></div><div>TransformExprs, TransformTemplateArguments, TransformObjCDictionaryLiteral get this right.</div><div><br></div><div>TransformFunctionTypeParams, TransformTypeTraitExpr, TransformLambdaScope get this wrong.</div>
<div><br></div><div>TransformSizeOfPackExpr gets this very wrong -- that's PR14858.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb">
<div class="h5">
>>> I'm not confident that this patch is correct, though: just stripping<br>
>>> off the PackExpansionType in the middle of instantiation seems weird.<br>
>>> Also, I'm not sure what other tests would be relevant.<br>
>><br>
>><br>
>> The patch only covers type template parameters; presumably, the same issue<br>
>> applies to non-type template parameters and template template parameters?<br>
><br>
> We have code which I'm pretty sure is equivalent for non-type template<br>
> parameters; see the beginning of<br>
> TemplateInstantiator::transformNonTypeTemplateParmRef.  The following<br>
> testcase works without any other modifications:<br>
><br>
> template <int x> struct DefaultValue { const int value = x;};<br>
> template <typename ... Args> struct tuple {};<br>
> template <int ... Args> using Zero = tuple<DefaultValue<Args> ...>;<br>
> template <int ... Args> void f(const Zero<Args ...> &t);<br>
> void f() {<br>
>     f(Zero<1,2,3>());<br>
> }<br>
><br>
> On the other hand, the following currently crashes:<br>
><br>
> template<int x> struct X {};<br>
> template <template<int x> class temp> struct DefaultValue { const<br>
> temp<0> value; };<br>
> template <typename ... Args> struct tuple {};<br>
> template <template<int x> class... Args> using Zero =<br>
> tuple<DefaultValue<Args> ...>;<br>
> template <template<int x> class... Args> void f(const Zero<Args ...> &t);<br>
> void f() {<br>
>     f(Zero<X,X,X>());<br>
> }<br>
><br>
> Conceptually, it's the same sort of fix, though.<br>
><br>
> I should probably use TemplateArgument::getPackExpansionPattern<br>
> consistently, instead of directly checking for a PackExpansionType.<br>
><br>
>> Does this correctly handle expanding the pack into a fixed set of parameters<br>
>> and a pack, in cases like:<br>
>><br>
>> template<typename ...Ts> struct S {};<br>
>> template<typename ...Ts> using X = S<S<Ts>...>;<br>
>> template<typename ...Ts> void f(X<int, Ts...> x);<br>
>> void g() { f(X<int, int, int>()); }<br>
>><br>
>> ? (I think it should, but this seems worth testing.)<br>
><br>
> It does.<br>
><br>
> Thanks for asking the right questions. I learned a lot more about my<br>
> patch by writing up the answers. :)<br>
</div></div></blockquote></div><br>