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

[Patrick Palka <ppalka@redhat.com>]

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.

-- >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);
-- 
2.39.2.501.gd9d677b2d8