Re: [PATCH 03/14] C++: add location_t wrapper nodes during parsing (minimal impl)

[David Malcolm <dmalcolm at redhat dot com>]

On Mon, 2017-12-11 at 21:10 -0500, Jason Merrill wrote:
> On 11/10/2017 04:45 PM, David Malcolm wrote:
> > The initial version of the patch kit added location wrapper nodes
> > around constants and uses-of-declarations, along with some other
> > places in the parser (typeid, alignof, sizeof, offsetof).
> > 
> > This version takes a much more minimal approach: it only adds
> > location wrapper nodes around the arguments at callsites, thus
> > not adding wrapper nodes around uses of constants and decls in
> > other
> > locations.
> > 
> > It keeps them for the other places in the parser (typeid, alignof,
> > sizeof, offsetof).
> > 
> > In addition, for now, each site that adds wrapper nodes is guarded
> > with !processing_template_decl, suppressing the creation of wrapper
> > nodes when processing template declarations.  This is to simplify
> > the patch kit so that we don't have to support wrapper nodes during
> > template expansion.
> 
> Hmm, it should be easy to support them, since NON_LVALUE_EXPR and 
> VIEW_CONVERT_EXPR don't otherwise appear in template trees.
> 
> Jason

I don't know if it's "easy"; it's at least non-trivial.

I attempted to support them in the obvious way by adding the two codes
to the switch statement tsubst_copy, reusing the case used by NOP_EXPR
and others, but ran into a issue when dealing with template parameter
packs.

Attached is the reproducer I've been testing with (minimized using
"delta" from a stdlib reproducer); my code was failing with:

../../src/cp-stdlib.ii: In instantiation of ‘struct allocator_traits<allocator<char> >’:
../../src/cp-stdlib.ii:31:8:   required from ‘struct __alloc_traits<allocator<char>, char>’
../../src/cp-stdlib.ii:43:75:   required from ‘class basic_string<char, allocator<char> >’
../../src/cp-stdlib.ii:47:58:   required from here
../../src/cp-stdlib.ii:27:55: sorry, unimplemented: use of ‘type_pack_expansion’ in template
     -> decltype(_S_construct(__a, __p, forward<_Args>(__args)...))  {   }
                                                       ^~~~~~

The issue is that normally "__args" would be a PARM_DECL of type
TYPE_PACK_EXPANSION, and that's handled by tsubst_decl, but on adding a
wrapper node we now have a VIEW_CONVERT_EXPR of the same type i.e.
TYPE_PACK_EXPANSION wrapping the PARM_DECL.

When tsubst traverses the tree, the VIEW_CONVERT_EXPR is reached first,
and it attempts to substitute the type TYPE_PACK_EXPANSION, which leads
to the "sorry".

If I understand things right, during substitution, only tsubst_decl on
PARM_DECL can handle nodes with type with code TYPE_PACK_EXPANSION.

The simplest approach seems to be to not create wrapper nodes for decls
of type TYPE_PACK_EXPANSION, and that seems to fix the issue.

Alternatively I can handle TYPE_PACK_EXPANSION for VIEW_CONVERT_EXPR in
tsubst by remapping the type to that of what they wrap after
substitution; doing so also fixes the issue.

Does this sound correct and sane?  Do you have a preferred approach?

On fixing that I'm running into a different issue when testing the
stdlib (again with parameter packs, I think), but I'm still
investigating that one...

Thanks
Dave

Attachment: cp-stdlib.ii
Description: Text document