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

[Jason Merrill <jason@redhat.com>]

On Thu, Dec 14, 2017 at 2:25 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> 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.

That does seem simplest.

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

This will be more correct.  For the wrappers you don't need all the
handling that we currently have for NOP_EXPR and such; since we know
they don't change the type, we can substitute what they wrap, and then
rewrap the result.

Jason