Re: [PATCH v2 1/2] c++: Standardise errors for module_may_redeclare

[Jason Merrill <jason@redhat.com>]

On 4/19/24 09:18, Nathaniel Shead wrote:
> On Mon, Apr 15, 2024 at 02:49:35PM +1000, Nathaniel Shead wrote:
>> I took another look at this patch and have split it into two, one (this
>> one) to standardise the error messages used and prepare
>> 'module_may_redeclare' for use with temploid friends, and another
>> followup patch to actually handle them correctly.
>>
>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>>
>> -- >8 --
>>
>> Currently different places calling 'module_may_redeclare' all emit very
>> similar but slightly different error messages, and handle different
>> kinds of declarations differently.  This patch makes the function
>> perform its own error messages so that they're all in one place, and
>> prepares it for use with temploid friends (PR c++/114275).
>>
>> gcc/cp/ChangeLog:
>>
>> 	* cp-tree.h (module_may_redeclare): Add default parameter.
>> 	* decl.cc (duplicate_decls): Don't emit errors for failed
>> 	module_may_redeclare.
>> 	(xref_tag): Likewise.
>> 	(start_enum): Likewise.
>> 	* semantics.cc (begin_class_definition): Likewise.
>> 	* module.cc (module_may_redeclare): Clean up logic. Emit error
>> 	messages on failure.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	* g++.dg/modules/enum-12.C: Update error message.
>> 	* g++.dg/modules/friend-5_b.C: Likewise.
>> 	* g++.dg/modules/shadow-1_b.C: Likewise.
>>
>> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
>> ---
>>   gcc/cp/cp-tree.h                          |   2 +-
>>   gcc/cp/decl.cc                            |  28 +----
>>   gcc/cp/module.cc                          | 120 ++++++++++++++--------
>>   gcc/cp/semantics.cc                       |   6 +-
>>   gcc/testsuite/g++.dg/modules/enum-12.C    |   2 +-
>>   gcc/testsuite/g++.dg/modules/friend-5_b.C |   2 +-
>>   gcc/testsuite/g++.dg/modules/shadow-1_b.C |   5 +-
>>   7 files changed, 89 insertions(+), 76 deletions(-)
>>
>> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
>> index 1dbb577a38d..faa7a0052a5 100644
>> --- a/gcc/cp/cp-tree.h
>> +++ b/gcc/cp/cp-tree.h
>> @@ -7401,7 +7401,7 @@ inline bool module_exporting_p ()
>>   
>>   extern module_state *get_module (tree name, module_state *parent = NULL,
>>   				 bool partition = false);
>> -extern bool module_may_redeclare (tree decl);
>> +extern bool module_may_redeclare (tree olddecl, tree newdecl = NULL);
>>   
>>   extern bool module_global_init_needed ();
>>   extern bool module_determine_import_inits ();
>> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
>> index 65ab64885ff..aa66da4829d 100644
>> --- a/gcc/cp/decl.cc
>> +++ b/gcc/cp/decl.cc
>> @@ -2279,18 +2279,8 @@ duplicate_decls (tree newdecl, tree olddecl, bool hiding, bool was_hidden)
>>         && TREE_CODE (olddecl) != NAMESPACE_DECL
>>         && !hiding)
>>       {
>> -      if (!module_may_redeclare (olddecl))
>> -	{
>> -	  if (DECL_ARTIFICIAL (olddecl))
>> -	    error ("declaration %qD conflicts with builtin", newdecl);
>> -	  else
>> -	    {
>> -	      error ("declaration %qD conflicts with import", newdecl);
>> -	      inform (olddecl_loc, "import declared %q#D here", olddecl);
>> -	    }
>> -
>> -	  return error_mark_node;
>> -	}
>> +      if (!module_may_redeclare (olddecl, newdecl))
>> +	return error_mark_node;
>>   
>>         tree not_tmpl = STRIP_TEMPLATE (olddecl);
>>         if (DECL_LANG_SPECIFIC (not_tmpl)
>> @@ -16620,12 +16610,7 @@ xref_tag (enum tag_types tag_code, tree name,
>>   	{
>>   	  tree decl = TYPE_NAME (t);
>>   	  if (!module_may_redeclare (decl))
>> -	    {
>> -	      auto_diagnostic_group d;
>> -	      error ("cannot declare %qD in a different module", decl);
>> -	      inform (DECL_SOURCE_LOCATION (decl), "previously declared here");
>> -	      return error_mark_node;
>> -	    }
>> +	    return error_mark_node;
>>   
>>   	  tree not_tmpl = STRIP_TEMPLATE (decl);
>>   	  if (DECL_LANG_SPECIFIC (not_tmpl)
>> @@ -16973,12 +16958,7 @@ start_enum (tree name, tree enumtype, tree underlying_type,
>>   	{
>>   	  tree decl = TYPE_NAME (enumtype);
>>   	  if (!module_may_redeclare (decl))
>> -	    {
>> -	      auto_diagnostic_group d;
>> -	      error ("cannot declare %qD in different module", decl);
>> -	      inform (DECL_SOURCE_LOCATION (decl), "previously declared here");
>> -	      enumtype = error_mark_node;
>> -	    }
>> +	    enumtype = error_mark_node;
>>   	  else
>>   	    set_instantiating_module (decl);
>>   	}
>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
>> index 001430a4a8f..e2d2910ae48 100644
>> --- a/gcc/cp/module.cc
>> +++ b/gcc/cp/module.cc
>> @@ -18992,11 +18992,15 @@ get_importing_module (tree decl, bool flexible)
>>     return module->mod;
>>   }
>>   
>> -/* Is it permissible to redeclare DECL.  */
>> +/* Is it permissible to redeclare OLDDECL with NEWDECL.
>> +
>> +   If NEWDECL is NULL, assumes that OLDDECL will be redeclared using
>> +   the current scope's module and attachment.  */
>>   
>>   bool
>> -module_may_redeclare (tree decl)
>> +module_may_redeclare (tree olddecl, tree newdecl)
>>   {
>> +  tree decl = olddecl;
>>     for (;;)
>>       {
>>         tree ctx = CP_DECL_CONTEXT (decl);
>> @@ -19010,58 +19014,94 @@ module_may_redeclare (tree decl)
>>         decl = TYPE_NAME (ctx);
>>       }
>>   
>> -  tree not_tmpl = STRIP_TEMPLATE (decl);
>> -
>>     int use_tpl = 0;
>> -  if (node_template_info (not_tmpl, use_tpl) && use_tpl)
>> +  if (node_template_info (STRIP_TEMPLATE (decl), use_tpl) && use_tpl)
>>       // Specializations of any kind can be redeclared anywhere.
>>       // FIXME: Should we be checking this in more places on the scope chain?
>>       return true;
>>   
>> -  if (!DECL_LANG_SPECIFIC (not_tmpl) || !DECL_MODULE_ATTACH_P (not_tmpl))
>> -    // Decl is attached to global module.  Current scope needs to be too.
>> -    return !module_attach_p ();
>> +  module_state *old_mod = (*modules)[0];
>> +  module_state *new_mod = old_mod;
>>   
>> -  module_state *me = (*modules)[0];
>> -  module_state *them = me;
>> +  tree old_origin = get_originating_module_decl (decl);
>> +  tree old_inner = STRIP_TEMPLATE (old_origin);
>> +  bool olddecl_attached_p = (DECL_LANG_SPECIFIC (old_inner)
>> +			     && DECL_MODULE_ATTACH_P (old_inner));
>> +  if (DECL_LANG_SPECIFIC (old_inner) && DECL_MODULE_IMPORT_P (old_inner))
>> +    {
>> +      unsigned index = import_entity_index (old_origin);
>> +      old_mod = import_entity_module (index);
>> +    }
>>   
>> -  if (DECL_LANG_SPECIFIC (not_tmpl) && DECL_MODULE_IMPORT_P (not_tmpl))
>> +  bool newdecl_attached_p = module_attach_p ();
>> +  if (newdecl)
>>       {
>> -      /* We can be given the TEMPLATE_RESULT.  We want the
>> -	 TEMPLATE_DECL.  */
>> -      int use_tpl = -1;
>> -      if (tree ti = node_template_info (decl, use_tpl))
>> +      tree new_origin = get_originating_module_decl (newdecl);
>> +      tree new_inner = STRIP_TEMPLATE (new_origin);
>> +      newdecl_attached_p = (DECL_LANG_SPECIFIC (new_inner)
>> +			    && DECL_MODULE_ATTACH_P (new_inner));
>> +      if (DECL_LANG_SPECIFIC (new_inner) && DECL_MODULE_IMPORT_P (new_inner))
>>   	{
>> -	  tree tmpl = TI_TEMPLATE (ti);
>> -	  if (use_tpl == 2)
>> -	    {
>> -	      /* A partial specialization.  Find that specialization's
>> -		 template_decl.  */
>> -	      for (tree list = DECL_TEMPLATE_SPECIALIZATIONS (tmpl);
>> -		   list; list = TREE_CHAIN (list))
>> -		if (DECL_TEMPLATE_RESULT (TREE_VALUE (list)) == decl)
>> -		  {
>> -		    decl = TREE_VALUE (list);
>> -		    break;
>> -		}
>> -	    }
>> -	  else if (DECL_TEMPLATE_RESULT (tmpl) == decl)
>> -	    decl = tmpl;
>> +	  unsigned index = import_entity_index (new_origin);
>> +	  new_mod = import_entity_module (index);
>>   	}
>> -      unsigned index = import_entity_index (decl);
>> -      them = import_entity_module (index);
>>       }
>>   
>> -  // Decl is attached to named module.  Current scope needs to be
>> -  // attaching to the same module.
>> -  if (!module_attach_p ())
>> -    return false;
>> +  /* Module attachment needs to match.  */
>> +  if (olddecl_attached_p == newdecl_attached_p)
>> +    {
>> +      if (!olddecl_attached_p)
>> +	/* Both are GM entities, OK.  */
>> +	return true;
>>   
>> -  // Both attached to named module.
>> -  if (me == them)
>> -    return true;
>> +      if (new_mod == old_mod
>> +	  || (new_mod && get_primary (new_mod) == get_primary (old_mod)))
>> +	/* Both attached to same named module, OK.  */
>> +	return true;
>> +    }
>> +
>> +  /* Attached to different modules, error.  */
>> +  decl = newdecl ? newdecl : olddecl;
>> +  location_t loc = newdecl ? DECL_SOURCE_LOCATION (newdecl) : input_location;
>> +  if (DECL_ARTIFICIAL (olddecl) && !DECL_IMPLICIT_TYPEDEF_P (olddecl))
>> +    error_at (loc, "declaration %qD conflicts with builtin", decl);
> 
> Or maybe this should be
> 
>    if (DECL_SOURCE_LOCATION (olddecl) == BUILTINS_LOCATION)
>      error_at (loc, "declaration %qD conflicts with builtin", decl);
> 
> and update the error message in enum-12.C?

Probably DECL_IS_UNDECLARED_BUILTIN?  OK with that change.

Jason