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
