On Fri, 2017-12-15 at 10:01 -0500, Jason Merrill wrote:
> On Thu, Dec 14, 2017 at 2:25 PM, David Malcolm <dmalc...@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.
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?
Thanks
Dave
