Re: [PATCH] c++: Support lambdas attached to FIELD_DECLs in modules [PR111710]

[Jason Merrill <jason@redhat.com>]

On 2/10/24 17:57, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> 
> Or should I re-introduce the tree checking and just add checks for the
> new kinds of declarations that could be have keyed decls?

This way is fine.

> -- >8 --
> 
> The fix for PR107398 weakened the restrictions that lambdas must belong
> to namespace scope. However this was not sufficient: we also need to
> allow lambdas keyed to FIELD_DECLs or PARM_DECLs.

I wonder about keying such lambdas to the class and function, 
respectively, rather than specifically to the field or parameter, but I 
suppose it doesn't matter.

OK with the comment adjustment below.

> Because this now requires 'DECL_MODULE_KEYED_DECLS_P' to be checked on a
> fairly large number of different kinds of DECLs, and that in general
> it's safe to just get 'false' as a result of a check on an unexpected
> DECL type, this patch also removes the tree checking from the accessor.
> 
> gcc/cp/ChangeLog:
> 
> 	* cp-tree.h (DECL_MODULE_KEYED_DECLS_P): Remove tree checking.
> 	(struct lang_decl_base): Update comments and fix whitespace.
> 	* module.cc (trees_out::lang_decl_bools): Always write
> 	module_keyed_decls_p flag...
> 	(trees_in::lang_decl_bools): ...and always read it.
> 	(trees_out::decl_value): Also handle keyed decls on decls other
> 	than VAR_DECL or FUNCTION_DECL.
> 	(trees_in::decl_value): Likewise.
> 	(trees_out::get_merge_kind): Likewise.
> 	(maybe_key_decl): Also handle lambdas attached to FIELD_DECLs
> 	and PARM_DECLS.
> 	(trees_out::key_mergeable): Likewise.
> 	(trees_in::key_mergeable): Support keyed decls in a TYPE_DECL
> 	container.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/lambda-7_a.C: New test.
> 	* g++.dg/modules/lambda-7_b.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/cp-tree.h                          | 23 ++++----
>   gcc/cp/module.cc                          | 70 +++++++++++------------
>   gcc/testsuite/g++.dg/modules/lambda-7_a.C | 20 +++++++
>   gcc/testsuite/g++.dg/modules/lambda-7_b.C | 16 ++++++
>   4 files changed, 81 insertions(+), 48 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/lambda-7_a.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/lambda-7_b.C
> 
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 334c11396c2..6ab82dc2d0f 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -1773,9 +1773,8 @@ check_constraint_info (tree t)
>     (DECL_LANG_SPECIFIC (DECL_MODULE_CHECK (NODE))->u.base.module_entity_p)
>   
>   /* DECL that has attached decls for ODR-relatedness.  */
> -#define DECL_MODULE_KEYED_DECLS_P(NODE)			\
> -  (DECL_LANG_SPECIFIC (TREE_CHECK2(NODE,FUNCTION_DECL,VAR_DECL))\
> -   ->u.base.module_keyed_decls_p)
> +#define DECL_MODULE_KEYED_DECLS_P(NODE) \
> +  (DECL_LANG_SPECIFIC (DECL_MODULE_CHECK (NODE))->u.base.module_keyed_decls_p)
>   
>   /* Whether this is an exported DECL.  Held on any decl that can appear
>      at namespace scope (function, var, type, template, const or
> @@ -2887,21 +2886,19 @@ struct GTY(()) lang_decl_base {
>     unsigned friend_or_tls : 1;		   /* var, fn, type or template */
>     unsigned unknown_bound_p : 1;		   /* var */
>     unsigned odr_used : 1;		   /* var or fn */
> -  unsigned concept_p : 1;                  /* applies to vars and functions */
> +  unsigned concept_p : 1;		   /* applies to vars and functions */
>     unsigned var_declared_inline_p : 1;	   /* var */
>     unsigned dependent_init_p : 1;	   /* var */
>   
>     /* The following apply to VAR, FUNCTION, TYPE, CONCEPT, & NAMESPACE
>        decls.  */

With your reformatting this comment now seems to apply to 
module_keyed_decls_p, which I don't think you mean.  Maybe just adjust 
this comment to say "the following 4"?

> -  unsigned module_purview_p : 1;	   // in named-module purview
> -  unsigned module_attach_p : 1;		   // attached to named module
> -  unsigned module_import_p : 1;     	   /* from an import */
> -  unsigned module_entity_p : 1;		   /* is in the entitity ary &
> -					      hash.  */
> -  /* VAR_DECL or FUNCTION_DECL has keyed decls.     */
> -  unsigned module_keyed_decls_p : 1;
> -
> -  /* 12 spare bits.  */
> +  unsigned module_purview_p : 1;	   /* in named-module purview */
> +  unsigned module_attach_p : 1;		   /* attached to named module */
> +  unsigned module_import_p : 1;		   /* from an import */
> +  unsigned module_entity_p : 1;		   /* is in the entitity ary & hash */
> +  unsigned module_keyed_decls_p : 1;	   /* has keys, applies to all decls */
> +
> +  /* 11 spare bits.  */
>   };
>   
>   /* True for DECL codes which have template info and access.  */
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 560d8f3b614..9742bca922c 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -5664,8 +5664,7 @@ trees_out::lang_decl_bools (tree t)
>        want to mark them as in module purview.  */
>     WB (lang->u.base.module_purview_p && !header_module_p ());
>     WB (lang->u.base.module_attach_p);
> -  if (VAR_OR_FUNCTION_DECL_P (t))
> -    WB (lang->u.base.module_keyed_decls_p);
> +  WB (lang->u.base.module_keyed_decls_p);
>     switch (lang->u.base.selector)
>       {
>       default:
> @@ -5738,8 +5737,7 @@ trees_in::lang_decl_bools (tree t)
>     RB (lang->u.base.dependent_init_p);
>     RB (lang->u.base.module_purview_p);
>     RB (lang->u.base.module_attach_p);
> -  if (VAR_OR_FUNCTION_DECL_P (t))
> -    RB (lang->u.base.module_keyed_decls_p);
> +  RB (lang->u.base.module_keyed_decls_p);
>     switch (lang->u.base.selector)
>       {
>       default:
> @@ -7869,8 +7867,7 @@ trees_out::decl_value (tree decl, depset *dep)
>         install_entity (decl, dep);
>       }
>   
> -  if (VAR_OR_FUNCTION_DECL_P (inner)
> -      && DECL_LANG_SPECIFIC (inner)
> +  if (DECL_LANG_SPECIFIC (inner)
>         && DECL_MODULE_KEYED_DECLS_P (inner)
>         && !is_key_order ())
>       {
> @@ -8170,8 +8167,7 @@ trees_in::decl_value ()
>     bool installed = install_entity (existing);
>     bool is_new = existing == decl;
>   
> -  if (VAR_OR_FUNCTION_DECL_P (inner)
> -      && DECL_LANG_SPECIFIC (inner)
> +  if (DECL_LANG_SPECIFIC (inner)
>         && DECL_MODULE_KEYED_DECLS_P (inner))
>       {
>         /* Read and maybe install the attached entities.  */
> @@ -10482,8 +10478,7 @@ trees_out::get_merge_kind (tree decl, depset *dep)
>   	      if (tree scope
>   		  = LAMBDA_EXPR_EXTRA_SCOPE (CLASSTYPE_LAMBDA_EXPR
>   					     (TREE_TYPE (decl))))
> -		if (TREE_CODE (scope) == VAR_DECL
> -		    && DECL_MODULE_KEYED_DECLS_P (scope))
> +		if (DECL_MODULE_KEYED_DECLS_P (scope))
>   		  {
>   		    mk = MK_keyed;
>   		    break;
> @@ -10787,7 +10782,9 @@ trees_out::key_mergeable (int tag, merge_kind mk, tree decl, tree inner,
>   	    gcc_checking_assert (LAMBDA_TYPE_P (TREE_TYPE (inner)));
>   	    tree scope = LAMBDA_EXPR_EXTRA_SCOPE (CLASSTYPE_LAMBDA_EXPR
>   						  (TREE_TYPE (inner)));
> -	    gcc_checking_assert (TREE_CODE (scope) == VAR_DECL);
> +	    gcc_checking_assert (TREE_CODE (scope) == VAR_DECL
> +				 || TREE_CODE (scope) == FIELD_DECL
> +				 || TREE_CODE (scope) == PARM_DECL);
>   	    auto *root = keyed_table->get (scope);
>   	    unsigned ix = root->length ();
>   	    /* If we don't find it, we'll write a really big number
> @@ -11065,6 +11062,26 @@ trees_in::key_mergeable (int tag, merge_kind mk, tree decl, tree inner,
>   		}
>   	    }
>   	}
> +      else if (mk == MK_keyed
> +	       && DECL_LANG_SPECIFIC (name)
> +	       && DECL_MODULE_KEYED_DECLS_P (name))
> +	{
> +	  gcc_checking_assert (TREE_CODE (container) == NAMESPACE_DECL
> +			       || TREE_CODE (container) == TYPE_DECL);
> +	  if (auto *set = keyed_table->get (name))
> +	    if (key.index < set->length ())
> +	      {
> +		existing = (*set)[key.index];
> +		if (existing)
> +		  {
> +		    gcc_checking_assert
> +		      (DECL_IMPLICIT_TYPEDEF_P (existing));
> +		    if (inner != decl)
> +		      existing
> +			= CLASSTYPE_TI_TEMPLATE (TREE_TYPE (existing));
> +		  }
> +	      }
> +	}
>         else
>   	switch (TREE_CODE (container))
>   	  {
> @@ -11072,27 +11089,8 @@ trees_in::key_mergeable (int tag, merge_kind mk, tree decl, tree inner,
>   	    gcc_unreachable ();
>   
>   	  case NAMESPACE_DECL:
> -	    if (mk == MK_keyed)
> -	      {
> -		if (DECL_LANG_SPECIFIC (name)
> -		    && VAR_OR_FUNCTION_DECL_P (name)
> -		    && DECL_MODULE_KEYED_DECLS_P (name))
> -		  if (auto *set = keyed_table->get (name))
> -		    if (key.index < set->length ())
> -		      {
> -			existing = (*set)[key.index];
> -			if (existing)
> -			  {
> -			    gcc_checking_assert
> -			      (DECL_IMPLICIT_TYPEDEF_P (existing));
> -			    if (inner != decl)
> -			      existing
> -				= CLASSTYPE_TI_TEMPLATE (TREE_TYPE (existing));
> -			  }
> -		      }
> -	      }
> -	    else if (is_attached
> -		     && !(state->is_module () || state->is_partition ()))
> +	    if (is_attached
> +		&& !(state->is_module () || state->is_partition ()))
>   	      kind = "unique";
>   	    else
>   	      {
> @@ -18981,9 +18979,11 @@ maybe_key_decl (tree ctx, tree decl)
>     if (!modules_p ())
>       return;
>   
> -  // FIXME: For now just deal with lambdas attached to var decls.
> -  // This might be sufficient?
> -  if (TREE_CODE (ctx) != VAR_DECL)
> +  /* We only need to deal with lambdas attached to var, field,
> +     or parm decls.  */
> +  if (TREE_CODE (ctx) != VAR_DECL
> +      && TREE_CODE (ctx) != FIELD_DECL
> +      && TREE_CODE (ctx) != PARM_DECL)
>       return;
>   
>     if (!keyed_table)
> diff --git a/gcc/testsuite/g++.dg/modules/lambda-7_a.C b/gcc/testsuite/g++.dg/modules/lambda-7_a.C
> new file mode 100644
> index 00000000000..289285cd926
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/lambda-7_a.C
> @@ -0,0 +1,20 @@
> +// { dg-additional-options "-fmodules-ts" }
> +// { dg-module-cmi foo }
> +
> +export module foo;
> +
> +export struct S {
> +  int (*a)(int) = [](int x) { return x * 2; };
> +
> +  int b(int x, int (*f)(int) = [](int x) { return x * 3; }) {
> +    return f(x);
> +  }
> +
> +  static int c(int x, int (*f)(int) = [](int x) { return x * 4; }) {
> +    return f(x);
> +  }
> +};
> +
> +export inline int d(int x, int (*f)(int) = [](int x) { return x * 5; }) {
> +  return f(x);
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/lambda-7_b.C b/gcc/testsuite/g++.dg/modules/lambda-7_b.C
> new file mode 100644
> index 00000000000..a8762399ee1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/lambda-7_b.C
> @@ -0,0 +1,16 @@
> +// { dg-module-do run }
> +// { dg-additional-options "-fmodules-ts" }
> +
> +import foo;
> +
> +int main() {
> +  S s;
> +  if (s.a(10) != 20)
> +    __builtin_abort();
> +  if (s.b(10) != 30)
> +    __builtin_abort();
> +  if (s.c(10) != 40)
> +    __builtin_abort();
> +  if (d(10) != 50)
> +    __builtin_abort();
> +}