Re: [PATCH] c++: merge tsubst_copy into tsubst_copy_and_build

[Jason Merrill <jason@redhat.com>]

On 10/4/23 12:08, Patrick Palka wrote:
> On Tue, 3 Oct 2023, Jason Merrill wrote:
> 
>> On 10/3/23 08:41, Patrick Palka wrote:
>>> On Mon, 2 Oct 2023, Patrick Palka wrote:
>>>
>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
>>>> OK for trunk?
>>>>
>>>> -- >8 --
>>>>
>>>> The relationship between tsubst_copy_and_build and tsubst_copy (two of
>>>> the main template argument substitution routines for expression trees)
>>>> is rather hazy.  The former is mostly a superset of the latter, with
>>>> some differences.
>>>>
>>>> The main difference is that they handle many tree codes differently, but
>>>> much of the tree code handling in tsubst_copy appears to be dead code[1].
>>>> This is because tsubst_copy only gets directly called in a few places
>>>> and mostly on id-expressions.  The interesting exceptions are PARM_DECL,
>>>> VAR_DECL, BIT_NOT_EXPR, SCOPE_REF, TEMPLATE_ID_EXPR and IDENTIFIER_NODE:
>>>>
>>>>    * for PARM_DECL and VAR_DECL, tsubst_copy_and_build calls tsubst_copy
>>>>      followed by doing some extra handling of its own
>>>>    * for BIT_NOT_EXPR tsubst_copy implicitly handles unresolved destructor
>>>>      calls (i.e. the first operand is an identifier or a type)
>>>>    * for SCOPE_REF, TEMPLATE_ID_EXPR and IDENTIFIER_NODE tsubst_copy
>>>>      refrains from doing name lookup of the terminal name
>>>>
>>>> Other more minor differences are that tsubst_copy exits early when
>>>> 'args' is null, and it calls maybe_dependent_member_ref
>>
>> That's curious, since what that function does seems like name lookup; I
>> wouldn't think we would want to call it when tf_no_name_lookup.
> 
> Ah, that makes sense I think.
> 
>>
>>>> and finally it dispatches to tsubst for type trees.
>>
>> And it looks like you fix the callers to avoid that?
> 
> Yes, I'll make a note of that in the commit message.
> 
>>
>>>> Thus tsubst_copy is (at this point) similar enough to
>>>> tsubst_copy_and_build
>>>> that it makes sense to merge the two functions, with the main difference
>>>> being the name lookup behavior[2].  So this patch merges tsubst_copy into
>>>> tsubst_copy_and_build via a new tsubst tf_no_name_lookup which controls
>>>> name lookup and resolution of a (top-level) id-expression.
>>>>
>>>> [1]: http://thrifty.mooo.com:8008/gcc-lcov/gcc/cp/pt.cc.gcov.html#17231
>>>> [2]: I don't know the history of tsubst_copy but I would guess it was
>>>> added before we settled on using processing_template_decl to control
>>>> whether our AST building routines perform semantic checking and return
>>>> non-templated trees, and so we needed a separate tsubst routine that
>>>> avoids semantic checking and always returns a templated tree for e.g.
>>>> partial substitution.
>>>
>>> Oops, this is wrong -- tsubst_copy_and_build came after tsubst_copy,
>>> and was introduced as an optimization with the intent of getting rid
>>> of tsubst_copy eventually:
>>> https://gcc.gnu.org/pipermail/gcc-patches/2003-January/093659.html
>>
>> I wonder if we want to add a small tsubst_name wrapper to call
>> tsubst_copy_and_build with tf_no_name_lookup?
> 
> Good idea, that'll complement tsubst_scope nicely.
> 
>>
>> Can we also merge in tsubst_expr and use that name instead of the unwieldy
>> tsubst_copy_and_build?
> 
> That'd be nice.  Another idea would be to rename tsubst_expr to
> tsubst_stmt and make it disjoint from tsubst_copy_and_build, and then
> rename tsubst_copy_and_build to tsubst_expr, to draw a distinction
> between statement-like trees (the substitution of which typically has
> side effects like calling add_stmt) and expression-like trees (which
> don't usually have such side effects).  I can work on that as a
> follow-up patch.
> 
> Here's v2 which guards the call to maybe_dependent_member_ref and adds
> tsubst_name, bootstrapped and regtested on x86_64-pc-linux-gnu:

OK.