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

[Jason Merrill <jason at redhat dot com>]

On Fri, Dec 15, 2017 at 11:35 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Fri, 2017-12-15 at 10:01 -0500, Jason Merrill wrote:
>> 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.
>
> (nods; I have this working)
>
> I've been debugging the other issues that I ran into when removing the
> "!processing_template_decl" filter on making wrapper nodes (ICEs and
> other errors on valid code).  They turn out to relate to wrappers
> around decls of type TEMPLATE_TYPE_PARM; having these wrappers leads to
> such VIEW_CONVERT_EXPRs turning up in unexpected places.

Hmm, that's odd.  What kind of decls?  A variable which happens to
have a template parameter for a type shouldn't be a problem.

> I could try to track all those places down, but it seems much simpler
> to just add an exclusion to adding wrapper nodes around decls of type
> TEMPLATE_TYPE_PARM.  On doing that my smoketests with the C++ stdlib
> work again.  Does that sound reasonable?

Jason