Ping for this. Or should this cleanup wait till GCC16?
On Wed, Feb 12, 2025 at 12:49:03AM +1100, Nathaniel Shead wrote:
> On Mon, Jan 27, 2025 at 10:20:05AM -0500, Patrick Palka wrote:
> > [snip]
> >
> > > @@ -18486,6 +18562,12 @@ dependent_operand_p (tree t)
> > > {
> > > while (TREE_CODE (t) == IMPLICIT_CONV_EXPR)
> > > t = TREE_OPERAND (t, 0);
> > > +
> > > + /* If we contain a TU_LOCAL_ENTITY assume we're non-dependent; we'll
> > > error
> > > + later when instantiating. */
> > > + if (expr_contains_tu_local_entity (t))
> > > + return false;
> >
> > I think it'd be more robust and cheaper (avoiding a separate tree walk)
> > to teach the general constexpr/dependence predicates about
> > TU_LOCAL_ENTITY instead of handling it only here.
> >
> > > +
> > > ++processing_template_decl;
> > > bool r = (potential_constant_expression (t)
> > > ? value_dependent_expression_p (t)
> > > @@ -20255,6 +20337,9 @@ tsubst_expr (tree t, tree args, tsubst_flags_t
> > > complain, tree in_decl)
> > > else
> > > object = NULL_TREE;
> > >
> > > + if (function_contains_tu_local_entity (templ))
> > > + RETURN (error_mark_node);
> > > +
> > > tree tid = lookup_template_function (templ, targs);
> > > protected_set_expr_location (tid, EXPR_LOCATION (t));
> > >
> > > @@ -20947,6 +21032,9 @@ tsubst_expr (tree t, tree args, tsubst_flags_t
> > > complain, tree in_decl)
> > > qualified_p = true;
> > > }
> > >
> > > + if (function_contains_tu_local_entity (function))
> > > + RETURN (error_mark_node);
> >
> > Similarly, maybe it'd suffice to check this more generally in the
> > OVERLOAD case of tsubst_expr?
> >
>
> So I'd completely missed the idea of handling it in the OVERLOAD case;
> doing this also fixes the issues I'd been having trying to handle it in
> potential_constant_expression. I think this should be a lot cleaner
> now.
>
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>
> -- >8 --
>
> Subject: [PATCH] c++: Handle TU_LOCAL_ENTITY in tsubst_expr and
> potential_constant_expression
>
> This cleans up the TU_LOCAL_ENTITY handling to avoid unnecessary
> tree walks and make the logic more robust.
>
> gcc/cp/ChangeLog:
>
> * constexpr.cc (potential_constant_expression_1): Handle
> TU_LOCAL_ENTITY.
> * pt.cc (expr_contains_tu_local_entity): Remove.
> (function_contains_tu_local_entity): Remove.
> (dependent_operand_p): Remove special handling for
> TU_LOCAL_ENTITY.
> (tsubst_expr): Handle TU_LOCAL_ENTITY when tsubsting OVERLOADs;
> remove now-unnecessary extra handling.
> (type_dependent_expression_p): Handle TU_LOCAL_ENTITY.
>
> Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> ---
> gcc/cp/constexpr.cc | 5 +++
> gcc/cp/pt.cc | 80 ++++++++-------------------------------------
> 2 files changed, 19 insertions(+), 66 deletions(-)
>
> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
> index f142dd32bc8..b36705fd4ce 100644
> --- a/gcc/cp/constexpr.cc
> +++ b/gcc/cp/constexpr.cc
> @@ -10825,6 +10825,11 @@ potential_constant_expression_1 (tree t, bool
> want_rval, bool strict, bool now,
> case CO_RETURN_EXPR:
> return false;
>
> + /* Assume a TU-local entity is not constant, we'll error later when
> + instantiating. */
> + case TU_LOCAL_ENTITY:
> + return false;
> +
> case NONTYPE_ARGUMENT_PACK:
> {
> tree args = ARGUMENT_PACK_ARGS (t);
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index f857b3f1180..966050a6608 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -9935,61 +9935,6 @@ complain_about_tu_local_entity (tree e)
> inform (TU_LOCAL_ENTITY_LOCATION (e), "declared here");
> }
>
> -/* Checks if T contains a TU-local entity. */
> -
> -static bool
> -expr_contains_tu_local_entity (tree t)
> -{
> - if (!modules_p ())
> - return false;
> -
> - auto walker = [](tree *tp, int *walk_subtrees, void *) -> tree
> - {
> - if (TREE_CODE (*tp) == TU_LOCAL_ENTITY)
> - return *tp;
> - if (!EXPR_P (*tp))
> - *walk_subtrees = false;
> - return NULL_TREE;
> - };
> - return cp_walk_tree (&t, walker, nullptr, nullptr);
> -}
> -
> -/* Errors and returns TRUE if X is a function that contains a TU-local
> - entity in its overload set. */
> -
> -static bool
> -function_contains_tu_local_entity (tree x)
> -{
> - if (!modules_p ())
> - return false;
> -
> - if (!x || x == error_mark_node)
> - return false;
> -
> - if (TREE_CODE (x) == OFFSET_REF
> - || TREE_CODE (x) == COMPONENT_REF)
> - x = TREE_OPERAND (x, 1);
> - x = MAYBE_BASELINK_FUNCTIONS (x);
> - if (TREE_CODE (x) == TEMPLATE_ID_EXPR)
> - x = TREE_OPERAND (x, 0);
> -
> - if (OVL_P (x))
> - for (tree ovl : lkp_range (x))
> - if (TREE_CODE (ovl) == TU_LOCAL_ENTITY)
> - {
> - x = ovl;
> - break;
> - }
> -
> - if (TREE_CODE (x) == TU_LOCAL_ENTITY)
> - {
> - complain_about_tu_local_entity (x);
> - return true;
> - }
> -
> - return false;
> -}
> -
> /* Return a TEMPLATE_ID_EXPR corresponding to the indicated FNS and
> ARGLIST. Valid choices for FNS are given in the cp-tree.def
> documentation for TEMPLATE_ID_EXPR. */
> @@ -18797,11 +18742,6 @@ dependent_operand_p (tree t)
> while (TREE_CODE (t) == IMPLICIT_CONV_EXPR)
> t = TREE_OPERAND (t, 0);
>
> - /* If we contain a TU_LOCAL_ENTITY assume we're non-dependent; we'll error
> - later when instantiating. */
> - if (expr_contains_tu_local_entity (t))
> - return false;
> -
> ++processing_template_decl;
> bool r = (potential_constant_expression (t)
> ? value_dependent_expression_p (t)
> @@ -20629,9 +20569,6 @@ tsubst_expr (tree t, tree args, tsubst_flags_t
> complain, tree in_decl)
> else
> object = NULL_TREE;
>
> - if (function_contains_tu_local_entity (templ))
> - RETURN (error_mark_node);
> -
> tree tid = lookup_template_function (templ, targs);
> protected_set_expr_location (tid, EXPR_LOCATION (t));
>
> @@ -21324,9 +21261,6 @@ tsubst_expr (tree t, tree args, tsubst_flags_t
> complain, tree in_decl)
> qualified_p = true;
> }
>
> - if (function_contains_tu_local_entity (function))
> - RETURN (error_mark_node);
> -
> nargs = call_expr_nargs (t);
> releasing_vec call_args;
> tsubst_call_args (t, args, complain, in_decl, call_args);
> @@ -22122,7 +22056,16 @@ tsubst_expr (tree t, tree args, tsubst_flags_t
> complain, tree in_decl)
> RETURN (t);
>
> case NAMESPACE_DECL:
> + RETURN (t);
> +
> case OVERLOAD:
> + if (modules_p ())
> + for (tree ovl : lkp_range (t))
> + if (TREE_CODE (ovl) == TU_LOCAL_ENTITY)
> + {
> + complain_about_tu_local_entity (ovl);
> + RETURN (error_mark_node);
> + }
> RETURN (t);
>
> case TEMPLATE_DECL:
> @@ -28990,6 +28933,11 @@ type_dependent_expression_p (tree expression)
>
> STRIP_ANY_LOCATION_WRAPPER (expression);
>
> + /* Assume a TU-local entity is not dependent, we'll error later when
> + instantiating anyway. */
> + if (TREE_CODE (expression) == TU_LOCAL_ENTITY)
> + return false;
> +
> /* An unresolved name is always dependent. */
> if (identifier_p (expression)
> || TREE_CODE (expression) == USING_DECL)
> --
> 2.47.0
>
