On Mon, 12 Aug 2024, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>
> I tried to implement a remapping of the slots for TARGET_EXPRs for the
> FIXME but I wasn't able to work out how to do so effectively. Given
> that I doubt this will be a common issue I felt probably easiest to
> leave it for now and focus on other issues in the meantime; thoughts?
>
> The other thing to note is that most of this function just has a single
> error message always indicated by a 'goto mismatch;' but I felt that it
> seemed reasonable to provide more specific error messages where we can.
> But given that in the long term we probably want to replace this
> function with an appropriately enhanced 'duplicate_decls' anyway maybe
> it's not worth worrying about; this patch is still useful in the
> meantime if only for the testcases, I hope.
>
> -- >8 --
>
> When merging a newly imported declaration with an existing declaration
> we don't currently propagate new default arguments, which causes issues
> when modularising header units. This patch adds logic to propagate
> default arguments to existing declarations on import, and error if the
> defaults do not match.
>
> PR c++/99274
>
> gcc/cp/ChangeLog:
>
> * module.cc (trees_in::is_matching_decl): Merge default
> arguments.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/modules/default-arg-1_a.H: New test.
> * g++.dg/modules/default-arg-1_b.C: New test.
> * g++.dg/modules/default-arg-2_a.H: New test.
> * g++.dg/modules/default-arg-2_b.C: New test.
> * g++.dg/modules/default-arg-3.h: New test.
> * g++.dg/modules/default-arg-3_a.H: New test.
> * g++.dg/modules/default-arg-3_b.C: New test.
>
> Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> ---
> gcc/cp/module.cc | 62 ++++++++++++++++++-
> .../g++.dg/modules/default-arg-1_a.H | 17 +++++
> .../g++.dg/modules/default-arg-1_b.C | 26 ++++++++
> .../g++.dg/modules/default-arg-2_a.H | 17 +++++
> .../g++.dg/modules/default-arg-2_b.C | 28 +++++++++
> gcc/testsuite/g++.dg/modules/default-arg-3.h | 13 ++++
> .../g++.dg/modules/default-arg-3_a.H | 5 ++
> .../g++.dg/modules/default-arg-3_b.C | 6 ++
> 8 files changed, 171 insertions(+), 3 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-1_a.H
> create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-1_b.C
> create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-2_a.H
> create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-2_b.C
> create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-3.h
> create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-3_a.H
> create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-3_b.C
>
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index f4d137b13a1..87f34bac578 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -11551,8 +11551,6 @@ trees_in::is_matching_decl (tree existing, tree decl,
> bool is_typedef)
>
> if (!same_type_p (TREE_VALUE (d_args), TREE_VALUE (e_args)))
> goto mismatch;
> -
> - // FIXME: Check default values
> }
>
> /* If EXISTING has an undeduced or uninstantiated exception
> @@ -11690,7 +11688,65 @@ trees_in::is_matching_decl (tree existing, tree
> decl, bool is_typedef)
> if (!DECL_EXTERNAL (d_inner))
> DECL_EXTERNAL (e_inner) = false;
>
> - // FIXME: Check default tmpl and fn parms here
> + if (TREE_CODE (decl) == TEMPLATE_DECL)
> + {
> + /* Merge default template arguments. */
> + tree d_parms = DECL_INNERMOST_TEMPLATE_PARMS (decl);
> + tree e_parms = DECL_INNERMOST_TEMPLATE_PARMS (existing);
> + gcc_checking_assert (TREE_VEC_LENGTH (d_parms)
> + == TREE_VEC_LENGTH (e_parms));
> + for (int i = 0; i < TREE_VEC_LENGTH (d_parms); ++i)
> + {
> + tree d_default = TREE_PURPOSE (TREE_VEC_ELT (d_parms, i));
> + tree& e_default = TREE_PURPOSE (TREE_VEC_ELT (e_parms, i));
> + if (e_default == NULL_TREE)
> + e_default = d_default;
> + else if (d_default != NULL_TREE
> + && !cp_tree_equal (d_default, e_default))
> + {
> + auto_diagnostic_group d;
> + tree d_parm = TREE_VALUE (TREE_VEC_ELT (d_parms, i));
> + tree e_parm = TREE_VALUE (TREE_VEC_ELT (e_parms, i));
> + error_at (DECL_SOURCE_LOCATION (d_parm),
> + "conflicting default argument for %#qD", d_parm);
> + inform (DECL_SOURCE_LOCATION (e_parm),
> + "existing default declared here");
> + return false;
> + }
> + }
> + }
FWIW for ordinary textual redeclarations we merge default template
arguments via merge_default_template_args. But the semantics are
slightly different on stream in because we want to allow a default
argument redefinition as long as it's cp_tree_equal, so I don't think
we can or want to use merge_default_template_args here.
> +
> + if (TREE_CODE (d_inner) == FUNCTION_DECL)
> + {
> + /* Merge default function arguments. */
> + tree d_parm = FUNCTION_FIRST_USER_PARMTYPE (d_inner);
> + tree e_parm = FUNCTION_FIRST_USER_PARMTYPE (e_inner);
> + int i = 0;
> + for (; d_parm && d_parm != void_list_node;
> + d_parm = TREE_CHAIN (d_parm), e_parm = TREE_CHAIN (e_parm), ++i)
> + {
> + tree d_default = TREE_PURPOSE (d_parm);
> + tree& e_default = TREE_PURPOSE (e_parm);
> + if (e_default == NULL_TREE)
> + e_default = d_default;
> + else if (d_default != NULL_TREE
> + && !cp_tree_equal (d_default, e_default)
> + /* FIXME: We need to remap the target to match the existing
> + parms so that cp_tree_equal works correctly, as with
> + break_out_target_exprs. */
Is the problem that cp_tree_equal expects TARGET_EXPR_SLOT to be
identical (despite the probably bitrotted special case for empty
DECL_NAME)? Maybe it'd make sense to always only compare
TARGET_EXPR_INITIAL, not TARGET_EXPR_SLOT..
The workaround seems fine with me though, LGTM.
> + && !(TREE_CODE (d_default) == TARGET_EXPR
> + && TREE_CODE (e_default) == TARGET_EXPR))
> + {
> + auto_diagnostic_group d;
> + error_at (get_fndecl_argument_location (d_inner, i),
> + "conflicting default argument for parameter %P of %#qD",
> + i, decl);
> + inform (get_fndecl_argument_location (e_inner, i),
> + "existing default declared here");
> + return false;
> + }
> + }
> + }
>
> return true;
> }
> diff --git a/gcc/testsuite/g++.dg/modules/default-arg-1_a.H
> b/gcc/testsuite/g++.dg/modules/default-arg-1_a.H
> new file mode 100644
> index 00000000000..65334930f74
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/default-arg-1_a.H
> @@ -0,0 +1,17 @@
> +// PR c++/99274
> +// { dg-additional-options "-fmodule-header" }
> +// { dg-module-cmi {} }
> +
> +void f(int a, int b = 123);
> +template <typename T> void g(T a, T b = {});
> +
> +template <typename T, typename U = int> struct A;
> +template <typename T, int N = 5> struct B;
> +
> +struct S {
> + template <typename T = int> void x();
> + void y(int n = 123);
> +};
> +
> +struct nontrivial { nontrivial(int); };
> +void h(nontrivial p = nontrivial(123));
> diff --git a/gcc/testsuite/g++.dg/modules/default-arg-1_b.C
> b/gcc/testsuite/g++.dg/modules/default-arg-1_b.C
> new file mode 100644
> index 00000000000..80822896de1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/default-arg-1_b.C
> @@ -0,0 +1,26 @@
> +// PR c++/99274
> +// { dg-additional-options "-fmodules-ts" }
> +// Propagate default args from import to existing decls
> +
> +void f(int a, int b);
> +template <typename T> void g(T a, T b);
> +template <typename T, typename U> struct A;
> +template <typename T, int N> struct B;
> +struct S {
> + template <typename T = int> void x();
> + void y(int n = 123);
> +};
> +struct nontrivial { nontrivial(int); };
> +void h(nontrivial p);
> +
> +import "default-arg-1_a.H";
> +
> +template <typename = A<int>, typename = B<int>> struct Q;
> +
> +int main() {
> + f(1);
> + g(2);
> + h();
> + S{}.x();
> + S{}.y();
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/default-arg-2_a.H
> b/gcc/testsuite/g++.dg/modules/default-arg-2_a.H
> new file mode 100644
> index 00000000000..085d4c7f792
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/default-arg-2_a.H
> @@ -0,0 +1,17 @@
> +// PR c++/99274
> +// { dg-additional-options "-fmodule-header" }
> +// { dg-module-cmi {} }
> +
> +void f(int a, int b = 123);
> +template <typename T> void g(T a, T b = 123);
> +
> +template <typename U = int> struct A;
> +template <int N = 123> struct B;
> +
> +struct S {
> + template <typename T = int> void x();
> + void y(int n = 123);
> +};
> +
> +struct nontrivial { nontrivial(int); };
> +void h(nontrivial p = nontrivial(123));
> diff --git a/gcc/testsuite/g++.dg/modules/default-arg-2_b.C
> b/gcc/testsuite/g++.dg/modules/default-arg-2_b.C
> new file mode 100644
> index 00000000000..a6cc58738c2
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/default-arg-2_b.C
> @@ -0,0 +1,28 @@
> +// PR c++/99274
> +// { dg-additional-options "-fmodules-ts -fno-module-lazy" }
> +// Check for conflicting defaults.
> +
> +void f(int a, int b = 456); // { dg-message "existing default declared
> here" }
> +
> +template <typename T>
> +void g(T a, T b = {}); // { dg-message "existing default declared here" }
> +
> +template <typename U = double> // { dg-message "existing default declared
> here" }
> +struct A;
> +
> +template <int N = 456> // { dg-message "existing default declared here" }
> +struct B;
> +
> +struct S {
> + template <typename T = double> // { dg-message "existing default declared
> here" }
> + void x();
> +
> + void y(int n = 456); // { dg-message "existing default declared here" }
> +};
> +
> +struct nontrivial { nontrivial(int); };
> +void h(nontrivial p = nontrivial(456)); // { dg-message "existing default
> declared here" "" { xfail *-*-* } }
> +
> +import "default-arg-2_a.H";
> +
> +// { dg-error "conflicting default argument" "" { target *-*-* } 0 }
> diff --git a/gcc/testsuite/g++.dg/modules/default-arg-3.h
> b/gcc/testsuite/g++.dg/modules/default-arg-3.h
> new file mode 100644
> index 00000000000..b55a6690f4a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/default-arg-3.h
> @@ -0,0 +1,13 @@
> +void f(int a, int b = 123);
> +template <typename T> void g(T a, T b = 123);
> +
> +template <typename U = int> struct A;
> +template <int N = 123> struct B;
> +
> +struct S {
> + template <typename T = int> void x();
> + void y(int n = 123);
> +};
> +
> +struct nontrivial { nontrivial(int); };
> +void h(nontrivial p = nontrivial(123));
> diff --git a/gcc/testsuite/g++.dg/modules/default-arg-3_a.H
> b/gcc/testsuite/g++.dg/modules/default-arg-3_a.H
> new file mode 100644
> index 00000000000..879bedce67f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/default-arg-3_a.H
> @@ -0,0 +1,5 @@
> +// PR c++/99274
> +// { dg-additional-options "-fmodule-header" }
> +// { dg-module-cmi {} }
> +
> +#include "default-arg-3.h"
> diff --git a/gcc/testsuite/g++.dg/modules/default-arg-3_b.C
> b/gcc/testsuite/g++.dg/modules/default-arg-3_b.C
> new file mode 100644
> index 00000000000..402dca6ff76
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/default-arg-3_b.C
> @@ -0,0 +1,6 @@
> +// PR c++/99274
> +// { dg-additional-options "-fmodules-ts -fno-module-lazy" }
> +// Don't complain about matching defaults.
> +
> +#include "default-arg-3.h"
> +import "default-arg-3_a.H";
> --
> 2.43.2
>
>
