Re: [PATCH] PR c++/53609 - Wrong argument deduction for pack expansion in argument pack

[Jason Merrill <jason at redhat dot com>]

Sorry it's taken so long for me to respond to this; I forgot about it 
over the holiday break.  The patch is in good shape now, just a few 
tweaks left:

On 12/19/2012 01:21 PM, Dodji Seketeli wrote:
> +      tree aps;			/* instance of ARGUMENT_PACK_SELECT
> +				   tree.  */

Odd comment formatting.

> +  /* We could not find any argument packs that work, so we'll just
> +     return an unsubstituted pack expansion.  The caller must be
> +     prepared to deal with this.  */

I still find this comment confusing.  I think it would be more correct 
to say "we'll just return a substituted pack expansion".

Also, it seems like the unsubstituted_packs code does what we want, so 
you could use that directly rather than change len.

> +  if (unsubstituted_packs || use_pack_expansion_extra)
>      {
> -      if (real_packs)
> +      if (use_pack_expansion_extra)

It seems like the outer "if" is redundant here.

> +  /*  If the Ith argument pack element is a pack expansion, then
> +      the Ith element resulting from the substituting is going to
> +      be a pack expansion as well.  */
> +  if (has_bare_parameter_packs (t))
> +    t = make_pack_expansion (t);

Instead of walking through 't' here, I think it would be cheaper to 
remember if the pack element was a pack expansion.  In which case maybe 
we don't need to split out has_bare_parameter_packs.

> +  if (len > 0)
>      {
> +      for (i = 0; i < len; ++i)
>  	{

The "if" seems redundant here, too.

Jason