Re: [PATCH 1/2] c++: factor out TYPENAME_TYPE substitution

[Jason Merrill <jason@redhat.com>]

On 2/21/23 19:05, Patrick Palka wrote:
> On Sun, 19 Feb 2023, Jason Merrill wrote:
> 
>> On 2/15/23 12:11, Patrick Palka wrote:
>>> On Wed, 15 Feb 2023, Jason Merrill wrote:
>>>
>>>> On 2/15/23 09:21, Patrick Palka wrote:
>>>>> On Tue, 14 Feb 2023, Jason Merrill wrote:
>>>>>
>>>>>> On 2/13/23 09:23, Patrick Palka wrote:
>>>>>>> [N.B. this is a corrected version of
>>>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/607443.html
>>>>>>> ]
>>>>>>>
>>>>>>> This patch factors out the TYPENAME_TYPE case of tsubst into a
>>>>>>> separate
>>>>>>> function tsubst_typename_type.  It also factors out the two tsubst
>>>>>>> flags
>>>>>>> controlling TYPENAME_TYPE substitution, tf_keep_type_decl and
>>>>>>> tf_tst_ok,
>>>>>>> into distinct boolean parameters of this new function (and of
>>>>>>> make_typename_type).  Consequently, callers which used to pass
>>>>>>> tf_tst_ok
>>>>>>> to tsubst now instead must directly call tsubst_typename_type when
>>>>>>> appropriate.
>>>>>>
>>>>>> Hmm, I don't love how that turns 4 lines into 8 more complex lines in
>>>>>> each
>>>>>> caller.  And the previous approach of saying "a CTAD placeholder is
>>>>>> OK"
>>>>>> seem
>>>>>> like better abstraction than repeating the specific TYPENAME_TYPE
>>>>>> handling
>>>>>> in
>>>>>> each place.
>>>>>
>>>>> Ah yeah, I see what you mean.  I was thinking since tf_tst_ok is
>>>>> specific to TYPENAME_TYPE handling and isn't propagated (i.e. it only
>>>>> affects top-level TYPENAME_TYPEs), it seemed cleaner to encode the flag
>>>>> as a bool parameter "template_ok" of tsubst_typename_type instead of as
>>>>> a global tsubst_flag that gets propagated freely.
>>>>>
>>>>>>
>>>>>>> In a subsequent patch we'll add another flag to
>>>>>>> tsubst_typename_type controlling whether we want to ignore non-types
>>>>>>> during the qualified lookup.
>>>>>
>>>>> As mentioned above, the second patch in this series would just add
>>>>> another flag "type_only" alongside "template_ok", since this flag will
>>>>> also only affects top-level TYPENAME_TYPEs and doesn't need to propagate
>>>>> like tsubst_flags.
>>>>>
>>>>> Except, it turns it, this new flag _does_ need to propagate, namely when
>>>>> expanding a variadic using:
>>>>>
>>>>>      using typename Ts::type::m...; // from typename25a.C below
>>>>>
>>>>> Here we have a USING_DECL whose USING_DECL_SCOPE is a
>>>>> TYPE_PACK_EXPANSION over TYPENAME_TYPE.  In order to correctly
>>>>> substitute this TYPENAME_TYPE, the USING_DECL case of tsubst_decl needs
>>>>> to pass an appropriate tsubst_flag to tsubst_pack_expansion to be
>>>>> propagated to tsubst (to be propagated to make_typename_type).
>>>>>
>>>>> So in light of this case it seems adding a new tsubst_flag is the
>>>>> way to go, which means we can avoid this refactoring patch entirely.
>>>>>
>>>>> Like so?  Bootstrapped and regtested on x86_64-pc-linux-gnu.
>>>>
>>>> OK, though I still wonder about adding a tsubst_scope function that would
>>>> add
>>>> the tf_qualifying_scope.
>>>
>>> Hmm, but we need to add tf_qualifying_scope to two tsubst_copy calls,
>>> one tsubst call and one tsubst_aggr_type call (with entering_scope=true).
>>> Would tsubst_scope call tsubst, tsubst_copy or tsubst_aggr_type?
>>
>> In general it would call tsubst.
>>
>> It's odd that anything is calling tsubst_copy with a type, that seems like a
>> copy/paste error.  But it just hands off to tsubst anyway, so the effect is
>> the same.
> 
> Ah, makes sense.  And if we make tsubst_copy hand off to tsubst
> immediately for TYPE_P trees we can make tsubst_copy oblivious to the
> tf_qualifying_scope flag.  I experimented with going a step further and
> fixing callers that pass a type to tsubst_copy, but that sort of clean
> up might be in vain given that we might be getting rid of tsubst_copy
> in the next stage 1.
> 
>>
>> tsubst_aggr_type is needed when pushing into the scope of a declarator; I
>> don't know offhand why we would need that when substituting the scope of a
>> TYPENAME_TYPE.
> 
> Apparently if we don't do entering_scope=true here then it breaks
> 
>    g++.dg/template/friend61.C
>    g++.dg/template/memfriend12.C
>    g++.dg/template/memfriend17.C
> 
> I think it's because without entering_scope=true, dependent substitution
> yields a dependent specialization A<T> instead of the primary template
> type A<T>, but we need the latter to perform qualified name lookup from
> such a substituted dependent scope.  I left that call alone for now.
> 
> How does this look?  Bootstrapped and regtested on x86_64-pc-linux-gnu.

OK.

> -- >8 --
> 
> Subject: [PATCH] c++: clean up tf_qualifying_scope usage
> 
> This patch introduces a convenience wrapper tsubst_scope for tsubst'ing
> into a type with tf_qualifying_scope set, and makes suitable callers use
> it instead of explicitly setting tf_qualifying_scope.  This also makes
> tsubst_copy immediately delegate to tsubst for all type trees, which
> allows tsubst_copy to be oblivious to the tf_qualifying_scope flag.
> 
> gcc/cp/ChangeLog:
> 
> 	* pt.cc (tsubst_scope): Define.
> 	(tsubst_decl) <case USING_DECL>: Call tsubst_scope instead of
> 	calling tsubst_scope with tf_qualifying_scope set.
> 	(tsubst_qualified_id): Call tsubst_scope instead of
> 	calling tsubst with tf_qualifying_scope set.
> 	(tsubst_copy): Immediately delegate to tsubst for all TYPE_P
> 	trees.  Remove tf_qualifying_scope manipulation.
> 	<case SCOPE_REF>: Call tsubst_scope instead of calling
> 	tsubst with tf_qualifying_scope set.
> ---
>   gcc/cp/pt.cc | 43 +++++++++++++++++--------------------------
>   1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index d11d540ab44..6a22fac5b32 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -206,6 +206,7 @@ static bool dependent_template_arg_p (tree);
>   static bool dependent_type_p_r (tree);
>   static tree tsubst_copy	(tree, tree, tsubst_flags_t, tree);
>   static tree tsubst_decl (tree, tree, tsubst_flags_t);
> +static tree tsubst_scope (tree, tree, tsubst_flags_t, tree);
>   static void perform_instantiation_time_access_checks (tree, tree);
>   static tree listify (tree);
>   static tree listify_autos (tree, tree);
> @@ -15004,9 +15005,7 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
>   	      variadic_p = true;
>   	    }
>   	  else
> -	    scope = tsubst_copy (scope, args,
> -				 complain | tf_qualifying_scope,
> -				 in_decl);
> +	    scope = tsubst_scope (scope, args, complain, in_decl);
>   
>   	  tree name = DECL_NAME (t);
>   	  if (IDENTIFIER_CONV_OP_P (name)
> @@ -16619,6 +16618,16 @@ tsubst (tree t, tree args, tsubst_flags_t complain, tree in_decl)
>       }
>   }
>   
> +/* Convenience wrapper over tsubst for substituting into the LHS
> +   of the :: scope resolution operator.  */
> +
> +static tree
> +tsubst_scope (tree t, tree args, tsubst_flags_t complain, tree in_decl)
> +{
> +  gcc_checking_assert (TYPE_P (t));
> +  return tsubst (t, args, complain | tf_qualifying_scope, in_decl);
> +}
> +
>   /* OLDFNS is a lookup set of member functions from some class template, and
>      NEWFNS is a lookup set of member functions from NEWTYPE, a specialization
>      of that class template.  Return the subset of NEWFNS which are
> @@ -16883,7 +16892,7 @@ tsubst_qualified_id (tree qualified_id, tree args,
>     scope = TREE_OPERAND (qualified_id, 0);
>     if (args)
>       {
> -      scope = tsubst (scope, args, complain | tf_qualifying_scope, in_decl);
> +      scope = tsubst_scope (scope, args, complain, in_decl);
>         expr = tsubst_copy (name, args, complain, in_decl);
>       }
>     else
> @@ -17129,8 +17138,8 @@ tsubst_copy (tree t, tree args, tsubst_flags_t complain, tree in_decl)
>     if (t == NULL_TREE || t == error_mark_node || args == NULL_TREE)
>       return t;
>   
> -  tsubst_flags_t qualifying_scope_flag = (complain & tf_qualifying_scope);
> -  complain &= ~tf_qualifying_scope;
> +  if (TYPE_P (t))
> +    return tsubst (t, args, complain, in_decl);
>   
>     if (tree d = maybe_dependent_member_ref (t, args, complain, in_decl))
>       return d;
> @@ -17605,8 +17614,7 @@ tsubst_copy (tree t, tree args, tsubst_flags_t complain, tree in_decl)
>   
>       case SCOPE_REF:
>         {
> -	tree op0 = tsubst_copy (TREE_OPERAND (t, 0), args,
> -				complain | tf_qualifying_scope, in_decl);
> +	tree op0 = tsubst_scope (TREE_OPERAND (t, 0), args, complain, in_decl);
>   	tree op1 = tsubst_copy (TREE_OPERAND (t, 1), args, complain, in_decl);
>   	return build_qualified_name (/*type=*/NULL_TREE, op0, op1,
>   				     QUALIFIED_NAME_IS_TEMPLATE (t));
> @@ -17702,26 +17710,9 @@ tsubst_copy (tree t, tree args, tsubst_flags_t complain, tree in_decl)
>   	return tree_cons (purpose, value, chain);
>         }
>   
> -    case RECORD_TYPE:
> -    case UNION_TYPE:
> -    case ENUMERAL_TYPE:
> -    case INTEGER_TYPE:
> -    case TEMPLATE_TYPE_PARM:
> -    case TEMPLATE_TEMPLATE_PARM:
> -    case BOUND_TEMPLATE_TEMPLATE_PARM:
>       case TEMPLATE_PARM_INDEX:
> -    case POINTER_TYPE:
> -    case REFERENCE_TYPE:
> -    case OFFSET_TYPE:
> -    case FUNCTION_TYPE:
> -    case METHOD_TYPE:
> -    case ARRAY_TYPE:
> -    case TYPENAME_TYPE:
> -    case UNBOUND_CLASS_TEMPLATE:
> -    case TYPEOF_TYPE:
> -    case DECLTYPE_TYPE:
>       case TYPE_DECL:
> -      return tsubst (t, args, complain | qualifying_scope_flag, in_decl);
> +      return tsubst (t, args, complain, in_decl);
>   
>       case USING_DECL:
>         t = DECL_NAME (t);