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 <nathanielosh...@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?
> + else if (DECL_LANG_SPECIFIC (old_inner) && DECL_MODULE_IMPORT_P
> (old_inner))
> + {
> + auto_diagnostic_group d;
> + if (newdecl_attached_p)
> + error_at (loc, "redeclaring %qD in module %qs conflicts with import",
> + decl, new_mod->get_flatname ());
> + else
> + error_at (loc, "redeclaring %qD in global module conflicts with import",
> + decl);
>
> - return me && get_primary (them) == get_primary (me);
> + if (olddecl_attached_p)
> + inform (DECL_SOURCE_LOCATION (olddecl),
> + "import declared attached to module %qs",
> + old_mod->get_flatname ());
> + else
> + inform (DECL_SOURCE_LOCATION (olddecl),
> + "import declared in global module");
> + }
> + else
> + {
> + auto_diagnostic_group d;
> + if (newdecl_attached_p)
> + error_at (loc, "conflicting declaration of %qD in module %qs",
> + decl, new_mod->get_flatname ());
> + else
> + error_at (loc, "conflicting declaration of %qD in global module",
> + decl);
> +
> + if (olddecl_attached_p)
> + inform (DECL_SOURCE_LOCATION (olddecl),
> + "previously declared in module %qs",
> + old_mod->get_flatname ());
> + else
> + inform (DECL_SOURCE_LOCATION (olddecl),
> + "previously declared in global module");
> + }
> + return false;
> }
>
> /* DECL is being created by this TU. Record it came from here. We
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 02c7c1bf5a4..2dde65a970b 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -3777,11 +3777,7 @@ begin_class_definition (tree t)
> if (modules_p ())
> {
> if (!module_may_redeclare (TYPE_NAME (t)))
> - {
> - error ("cannot declare %qD in a different module", TYPE_NAME (t));
> - inform (DECL_SOURCE_LOCATION (TYPE_NAME (t)), "declared here");
> - return error_mark_node;
> - }
> + return error_mark_node;
> set_instantiating_module (TYPE_NAME (t));
> set_defining_module (TYPE_NAME (t));
> }
> diff --git a/gcc/testsuite/g++.dg/modules/enum-12.C
> b/gcc/testsuite/g++.dg/modules/enum-12.C
> index 57eeb85d92a..019c3da4218 100644
> --- a/gcc/testsuite/g++.dg/modules/enum-12.C
> +++ b/gcc/testsuite/g++.dg/modules/enum-12.C
> @@ -4,7 +4,7 @@
>
> export module foo;
> namespace std {
> - enum class align_val_t : decltype(sizeof(int)) {}; // { dg-error
> "different module" }
> + enum class align_val_t : decltype(sizeof(int)) {}; // { dg-error
> "conflicting declaration" }
> }
>
> // { dg-prune-output "not writing module" }
> diff --git a/gcc/testsuite/g++.dg/modules/friend-5_b.C
> b/gcc/testsuite/g++.dg/modules/friend-5_b.C
> index f043d7a340d..6b561265155 100644
> --- a/gcc/testsuite/g++.dg/modules/friend-5_b.C
> +++ b/gcc/testsuite/g++.dg/modules/friend-5_b.C
> @@ -4,7 +4,7 @@
> export module bar;
> import foo;
>
> -class B { // { dg-error "in a different module" }
> +class B { // { dg-error "conflicts with import" }
> B() { object.value = 42; }
> A object;
> };
> diff --git a/gcc/testsuite/g++.dg/modules/shadow-1_b.C
> b/gcc/testsuite/g++.dg/modules/shadow-1_b.C
> index 646381237ac..7f6a3182998 100644
> --- a/gcc/testsuite/g++.dg/modules/shadow-1_b.C
> +++ b/gcc/testsuite/g++.dg/modules/shadow-1_b.C
> @@ -1,8 +1,5 @@
> // { dg-additional-options -fmodules-ts }
> import shadow;
>
> -// unfortunately not the exact same diagnostic in both cases :(
> -
> void stat (); // { dg-error "conflicts with import" }
> -
> -struct stat {}; // { dg-error "in a different module" }
> +struct stat {}; // { dg-error "conflicts with import" }
> --
> 2.43.2
>
