| Message ID | 66b9b2da.050a0220.2e8a84.8a49@mx.google.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++/modules: Merge default arguments [PR99274] | expand |
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 <nathanieloshead@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 > >
On Thu, Aug 22, 2024 at 02:20:14PM -0400, Patrick Palka wrote: > 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 <nathanieloshead@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.. > It's actually the opposite: the special case for empty DECL_NAME does kick in here and we proceed to just comparing the TARGET_EXPR_INITIAL. But for e.g. the 'void h(nontrivial p = nontrivial(123));' case the contents of the slot is an AGGR_INIT_EXPR which depends on the variable declared in the TARGET_EXPR_SLOT. So in this case within the default-arg-3 testcase, d_default is (some details elided): <target_expr 0x7ffff74ea7e0 type <record_type 0x7ffff76b70a8 nontrivial ...> side-effects tree_0 arg:0 <var_decl 0x7ffff76bb000 D.2886 type <record_type 0x7ffff76b70a8 nontrivial> ...> arg:1 <aggr_init_expr 0x7ffff76b1f40 type <void_type 0x7ffff7512f18 void ...> side-effects tree_0 arg:0 <integer_cst 0x7ffff76ae468 constant 5> arg:1 <addr_expr 0x7ffff76ba220 type <pointer_type 0x7ffff76b7b28> constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp >> arg:2 <var_decl 0x7ffff76bb000 D.2886> arg:3 <convert_expr 0x7ffff76ba240 type <pointer_type 0x7ffff76b71f8> ...> arg:4 <non_lvalue_expr 0x7ffff76ba260 type <integer_type 0x7ffff75125e8 int> constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123> ... While e_default is <target_expr 0x7ffff74ea7a8 type <record_type 0x7ffff76b70a8 nontrivial ...> side-effects tree_0 arg:0 <var_decl 0x7ffff74faea0 D.2860 type <record_type 0x7ffff76b70a8 nontrivial> ...> arg:1 <aggr_init_expr 0x7ffff76b1ac0 type <void_type 0x7ffff7512f18 void ...> side-effects tree_0 arg:0 <integer_cst 0x7ffff76ae468 constant 5> arg:1 <addr_expr 0x7ffff76ba000 type <pointer_type 0x7ffff76b7b28> constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp >> arg:2 <var_decl 0x7ffff74faea0 D.2860> arg:3 <convert_expr 0x7ffff7692e80 type <pointer_type 0x7ffff76b71f8> ...> arg:4 <non_lvalue_expr 0x7ffff7692e60 type <integer_type 0x7ffff75125e8 int> constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123> ... -- note the use of 'D.2860' vs. 'D.2886' here in arg:1 of the AGGR_INIT_EXPR, which causes cp_tree_equal to decide they're different. I imagine probably we would need to merge the VAR_DECLs here but in the presence of nested TARGET_EXPRs that started to get hard to figure out, and at some point I found I was just reimplementing a limited version of cp_tree_equal with an additional map so I decided to punt on it for now. Maybe a better option then would be to add a parameter to cp_tree_equal with an optional hash map that it could use to treat certain decls as 'equal' (even if they weren't), used within this special handling for TARGET_EXPRs, but that seems overkill for what is ultimately a pretty rare error case IMO. > 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 > > > > >
Ping. On Fri, Aug 23, 2024 at 10:02:44AM +1000, Nathaniel Shead wrote: > On Thu, Aug 22, 2024 at 02:20:14PM -0400, Patrick Palka wrote: > > 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 <nathanieloshead@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.. > > > > It's actually the opposite: the special case for empty DECL_NAME does > kick in here and we proceed to just comparing the TARGET_EXPR_INITIAL. > But for e.g. the 'void h(nontrivial p = nontrivial(123));' case the > contents of the slot is an AGGR_INIT_EXPR which depends on the variable > declared in the TARGET_EXPR_SLOT. > > So in this case within the default-arg-3 testcase, d_default is > (some details elided): > > <target_expr 0x7ffff74ea7e0 > type <record_type 0x7ffff76b70a8 nontrivial ...> > side-effects tree_0 > arg:0 <var_decl 0x7ffff76bb000 D.2886 type <record_type 0x7ffff76b70a8 nontrivial> ...> > arg:1 <aggr_init_expr 0x7ffff76b1f40 > type <void_type 0x7ffff7512f18 void ...> > side-effects tree_0 > arg:0 <integer_cst 0x7ffff76ae468 constant 5> > arg:1 <addr_expr 0x7ffff76ba220 type <pointer_type 0x7ffff76b7b28> > constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp >> arg:2 <var_decl 0x7ffff76bb000 D.2886> > arg:3 <convert_expr 0x7ffff76ba240 type <pointer_type 0x7ffff76b71f8> ...> > arg:4 <non_lvalue_expr 0x7ffff76ba260 type <integer_type 0x7ffff75125e8 int> > constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123> ... > > While e_default is > > <target_expr 0x7ffff74ea7a8 > type <record_type 0x7ffff76b70a8 nontrivial ...> > side-effects tree_0 > arg:0 <var_decl 0x7ffff74faea0 D.2860 type <record_type 0x7ffff76b70a8 nontrivial> ...> > arg:1 <aggr_init_expr 0x7ffff76b1ac0 > type <void_type 0x7ffff7512f18 void ...> > side-effects tree_0 > arg:0 <integer_cst 0x7ffff76ae468 constant 5> > arg:1 <addr_expr 0x7ffff76ba000 type <pointer_type 0x7ffff76b7b28> > constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp >> arg:2 <var_decl 0x7ffff74faea0 D.2860> > arg:3 <convert_expr 0x7ffff7692e80 type <pointer_type 0x7ffff76b71f8> ...> > arg:4 <non_lvalue_expr 0x7ffff7692e60 type <integer_type 0x7ffff75125e8 int> > constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123> ... > > -- note the use of 'D.2860' vs. 'D.2886' here in arg:1 of the > AGGR_INIT_EXPR, which causes cp_tree_equal to decide they're different. > I imagine probably we would need to merge the VAR_DECLs here but in the > presence of nested TARGET_EXPRs that started to get hard to figure out, > and at some point I found I was just reimplementing a limited version of > cp_tree_equal with an additional map so I decided to punt on it for now. > > Maybe a better option then would be to add a parameter to cp_tree_equal > with an optional hash map that it could use to treat certain decls as > 'equal' (even if they weren't), used within this special handling for > TARGET_EXPRs, but that seems overkill for what is ultimately a pretty > rare error case IMO. > > > 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 > > > > > > > >
On Fri, 23 Aug 2024, Nathaniel Shead wrote: > On Thu, Aug 22, 2024 at 02:20:14PM -0400, Patrick Palka wrote: > > 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 <nathanieloshead@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.. > > > > It's actually the opposite: the special case for empty DECL_NAME does > kick in here and we proceed to just comparing the TARGET_EXPR_INITIAL. > But for e.g. the 'void h(nontrivial p = nontrivial(123));' case the > contents of the slot is an AGGR_INIT_EXPR which depends on the variable > declared in the TARGET_EXPR_SLOT. > > So in this case within the default-arg-3 testcase, d_default is > (some details elided): > > <target_expr 0x7ffff74ea7e0 > type <record_type 0x7ffff76b70a8 nontrivial ...> > side-effects tree_0 > arg:0 <var_decl 0x7ffff76bb000 D.2886 type <record_type 0x7ffff76b70a8 nontrivial> ...> > arg:1 <aggr_init_expr 0x7ffff76b1f40 > type <void_type 0x7ffff7512f18 void ...> > side-effects tree_0 > arg:0 <integer_cst 0x7ffff76ae468 constant 5> > arg:1 <addr_expr 0x7ffff76ba220 type <pointer_type 0x7ffff76b7b28> > constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp >> arg:2 <var_decl 0x7ffff76bb000 D.2886> > arg:3 <convert_expr 0x7ffff76ba240 type <pointer_type 0x7ffff76b71f8> ...> > arg:4 <non_lvalue_expr 0x7ffff76ba260 type <integer_type 0x7ffff75125e8 int> > constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123> ... > > While e_default is > > <target_expr 0x7ffff74ea7a8 > type <record_type 0x7ffff76b70a8 nontrivial ...> > side-effects tree_0 > arg:0 <var_decl 0x7ffff74faea0 D.2860 type <record_type 0x7ffff76b70a8 nontrivial> ...> > arg:1 <aggr_init_expr 0x7ffff76b1ac0 > type <void_type 0x7ffff7512f18 void ...> > side-effects tree_0 > arg:0 <integer_cst 0x7ffff76ae468 constant 5> > arg:1 <addr_expr 0x7ffff76ba000 type <pointer_type 0x7ffff76b7b28> > constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp >> arg:2 <var_decl 0x7ffff74faea0 D.2860> > arg:3 <convert_expr 0x7ffff7692e80 type <pointer_type 0x7ffff76b71f8> ...> > arg:4 <non_lvalue_expr 0x7ffff7692e60 type <integer_type 0x7ffff75125e8 int> > constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123> ... > > -- note the use of 'D.2860' vs. 'D.2886' here in arg:1 of the > AGGR_INIT_EXPR, which causes cp_tree_equal to decide they're different. > I imagine probably we would need to merge the VAR_DECLs here but in the > presence of nested TARGET_EXPRs that started to get hard to figure out, > and at some point I found I was just reimplementing a limited version of > cp_tree_equal with an additional map so I decided to punt on it for now. > > Maybe a better option then would be to add a parameter to cp_tree_equal > with an optional hash map that it could use to treat certain decls as > 'equal' (even if they weren't), used within this special handling for > TARGET_EXPRs, but that seems overkill for what is ultimately a pretty > rare error case IMO. Ah, makes sense, agreed FWIW > > > 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 > > > > > > > > > >
Ping for https://gcc.gnu.org/pipermail/gcc-patches/2024-August/660134.html. On Thu, Sep 12, 2024 at 01:35:38PM -0400, Patrick Palka wrote: > On Fri, 23 Aug 2024, Nathaniel Shead wrote: > > > On Thu, Aug 22, 2024 at 02:20:14PM -0400, Patrick Palka wrote: > > > 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 <nathanieloshead@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.. > > > > > > > It's actually the opposite: the special case for empty DECL_NAME does > > kick in here and we proceed to just comparing the TARGET_EXPR_INITIAL. > > But for e.g. the 'void h(nontrivial p = nontrivial(123));' case the > > contents of the slot is an AGGR_INIT_EXPR which depends on the variable > > declared in the TARGET_EXPR_SLOT. > > > > So in this case within the default-arg-3 testcase, d_default is > > (some details elided): > > > > <target_expr 0x7ffff74ea7e0 > > type <record_type 0x7ffff76b70a8 nontrivial ...> > > side-effects tree_0 > > arg:0 <var_decl 0x7ffff76bb000 D.2886 type <record_type 0x7ffff76b70a8 nontrivial> ...> > > arg:1 <aggr_init_expr 0x7ffff76b1f40 > > type <void_type 0x7ffff7512f18 void ...> > > side-effects tree_0 > > arg:0 <integer_cst 0x7ffff76ae468 constant 5> > > arg:1 <addr_expr 0x7ffff76ba220 type <pointer_type 0x7ffff76b7b28> > > constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp >> arg:2 <var_decl 0x7ffff76bb000 D.2886> > > arg:3 <convert_expr 0x7ffff76ba240 type <pointer_type 0x7ffff76b71f8> ...> > > arg:4 <non_lvalue_expr 0x7ffff76ba260 type <integer_type 0x7ffff75125e8 int> > > constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123> ... > > > > While e_default is > > > > <target_expr 0x7ffff74ea7a8 > > type <record_type 0x7ffff76b70a8 nontrivial ...> > > side-effects tree_0 > > arg:0 <var_decl 0x7ffff74faea0 D.2860 type <record_type 0x7ffff76b70a8 nontrivial> ...> > > arg:1 <aggr_init_expr 0x7ffff76b1ac0 > > type <void_type 0x7ffff7512f18 void ...> > > side-effects tree_0 > > arg:0 <integer_cst 0x7ffff76ae468 constant 5> > > arg:1 <addr_expr 0x7ffff76ba000 type <pointer_type 0x7ffff76b7b28> > > constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp >> arg:2 <var_decl 0x7ffff74faea0 D.2860> > > arg:3 <convert_expr 0x7ffff7692e80 type <pointer_type 0x7ffff76b71f8> ...> > > arg:4 <non_lvalue_expr 0x7ffff7692e60 type <integer_type 0x7ffff75125e8 int> > > constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123> ... > > > > -- note the use of 'D.2860' vs. 'D.2886' here in arg:1 of the > > AGGR_INIT_EXPR, which causes cp_tree_equal to decide they're different. > > I imagine probably we would need to merge the VAR_DECLs here but in the > > presence of nested TARGET_EXPRs that started to get hard to figure out, > > and at some point I found I was just reimplementing a limited version of > > cp_tree_equal with an additional map so I decided to punt on it for now. > > > > Maybe a better option then would be to add a parameter to cp_tree_equal > > with an optional hash map that it could use to treat certain decls as > > 'equal' (even if they weren't), used within this special handling for > > TARGET_EXPRs, but that seems overkill for what is ultimately a pretty > > rare error case IMO. > > Ah, makes sense, agreed FWIW > > > > > > 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 > > > > > > > > > > > > > > > >
Ping for https://gcc.gnu.org/pipermail/gcc-patches/2024-August/660134.html. On Thu, Sep 12, 2024 at 01:35:38PM -0400, Patrick Palka wrote: > On Fri, 23 Aug 2024, Nathaniel Shead wrote: > > > On Thu, Aug 22, 2024 at 02:20:14PM -0400, Patrick Palka wrote: > > > 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 <nathanieloshead@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.. > > > > > > > It's actually the opposite: the special case for empty DECL_NAME does > > kick in here and we proceed to just comparing the TARGET_EXPR_INITIAL. > > But for e.g. the 'void h(nontrivial p = nontrivial(123));' case the > > contents of the slot is an AGGR_INIT_EXPR which depends on the variable > > declared in the TARGET_EXPR_SLOT. > > > > So in this case within the default-arg-3 testcase, d_default is > > (some details elided): > > > > <target_expr 0x7ffff74ea7e0 > > type <record_type 0x7ffff76b70a8 nontrivial ...> > > side-effects tree_0 > > arg:0 <var_decl 0x7ffff76bb000 D.2886 type <record_type 0x7ffff76b70a8 nontrivial> ...> > > arg:1 <aggr_init_expr 0x7ffff76b1f40 > > type <void_type 0x7ffff7512f18 void ...> > > side-effects tree_0 > > arg:0 <integer_cst 0x7ffff76ae468 constant 5> > > arg:1 <addr_expr 0x7ffff76ba220 type <pointer_type 0x7ffff76b7b28> > > constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp >> arg:2 <var_decl 0x7ffff76bb000 D.2886> > > arg:3 <convert_expr 0x7ffff76ba240 type <pointer_type 0x7ffff76b71f8> ...> > > arg:4 <non_lvalue_expr 0x7ffff76ba260 type <integer_type 0x7ffff75125e8 int> > > constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123> ... > > > > While e_default is > > > > <target_expr 0x7ffff74ea7a8 > > type <record_type 0x7ffff76b70a8 nontrivial ...> > > side-effects tree_0 > > arg:0 <var_decl 0x7ffff74faea0 D.2860 type <record_type 0x7ffff76b70a8 nontrivial> ...> > > arg:1 <aggr_init_expr 0x7ffff76b1ac0 > > type <void_type 0x7ffff7512f18 void ...> > > side-effects tree_0 > > arg:0 <integer_cst 0x7ffff76ae468 constant 5> > > arg:1 <addr_expr 0x7ffff76ba000 type <pointer_type 0x7ffff76b7b28> > > constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp >> arg:2 <var_decl 0x7ffff74faea0 D.2860> > > arg:3 <convert_expr 0x7ffff7692e80 type <pointer_type 0x7ffff76b71f8> ...> > > arg:4 <non_lvalue_expr 0x7ffff7692e60 type <integer_type 0x7ffff75125e8 int> > > constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123> ... > > > > -- note the use of 'D.2860' vs. 'D.2886' here in arg:1 of the > > AGGR_INIT_EXPR, which causes cp_tree_equal to decide they're different. > > I imagine probably we would need to merge the VAR_DECLs here but in the > > presence of nested TARGET_EXPRs that started to get hard to figure out, > > and at some point I found I was just reimplementing a limited version of > > cp_tree_equal with an additional map so I decided to punt on it for now. > > > > Maybe a better option then would be to add a parameter to cp_tree_equal > > with an optional hash map that it could use to treat certain decls as > > 'equal' (even if they weren't), used within this special handling for > > TARGET_EXPRs, but that seems overkill for what is ultimately a pretty > > rare error case IMO. > > Ah, makes sense, agreed FWIW > > > > > > 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 > > > > > > > > > > > > > > > >
On 8/22/24 8:02 PM, Nathaniel Shead wrote: > On Thu, Aug 22, 2024 at 02:20:14PM -0400, Patrick Palka wrote: >> 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 <nathanieloshead@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.. >> > > It's actually the opposite: the special case for empty DECL_NAME does > kick in here and we proceed to just comparing the TARGET_EXPR_INITIAL. > But for e.g. the 'void h(nontrivial p = nontrivial(123));' case the > contents of the slot is an AGGR_INIT_EXPR which depends on the variable > declared in the TARGET_EXPR_SLOT. > > So in this case within the default-arg-3 testcase, d_default is > (some details elided): > > <target_expr 0x7ffff74ea7e0 > type <record_type 0x7ffff76b70a8 nontrivial ...> > side-effects tree_0 > arg:0 <var_decl 0x7ffff76bb000 D.2886 type <record_type 0x7ffff76b70a8 nontrivial> ...> > arg:1 <aggr_init_expr 0x7ffff76b1f40 > type <void_type 0x7ffff7512f18 void ...> > side-effects tree_0 > arg:0 <integer_cst 0x7ffff76ae468 constant 5> > arg:1 <addr_expr 0x7ffff76ba220 type <pointer_type 0x7ffff76b7b28> > constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp >> arg:2 <var_decl 0x7ffff76bb000 D.2886> > arg:3 <convert_expr 0x7ffff76ba240 type <pointer_type 0x7ffff76b71f8> ...> > arg:4 <non_lvalue_expr 0x7ffff76ba260 type <integer_type 0x7ffff75125e8 int> > constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123> ... > > While e_default is > > <target_expr 0x7ffff74ea7a8 > type <record_type 0x7ffff76b70a8 nontrivial ...> > side-effects tree_0 > arg:0 <var_decl 0x7ffff74faea0 D.2860 type <record_type 0x7ffff76b70a8 nontrivial> ...> > arg:1 <aggr_init_expr 0x7ffff76b1ac0 > type <void_type 0x7ffff7512f18 void ...> > side-effects tree_0 > arg:0 <integer_cst 0x7ffff76ae468 constant 5> > arg:1 <addr_expr 0x7ffff76ba000 type <pointer_type 0x7ffff76b7b28> > constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp >> arg:2 <var_decl 0x7ffff74faea0 D.2860> > arg:3 <convert_expr 0x7ffff7692e80 type <pointer_type 0x7ffff76b71f8> ...> > arg:4 <non_lvalue_expr 0x7ffff7692e60 type <integer_type 0x7ffff75125e8 int> > constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123> ... > > -- note the use of 'D.2860' vs. 'D.2886' here in arg:1 of the > AGGR_INIT_EXPR, which causes cp_tree_equal to decide they're different. > I imagine probably we would need to merge the VAR_DECLs here but in the > presence of nested TARGET_EXPRs that started to get hard to figure out, > and at some point I found I was just reimplementing a limited version of > cp_tree_equal with an additional map so I decided to punt on it for now. > > Maybe a better option then would be to add a parameter to cp_tree_equal > with an optional hash map that it could use to treat certain decls as > 'equal' (even if they weren't), used within this special handling for > TARGET_EXPRs, but that seems overkill for what is ultimately a pretty > rare error case IMO. Sounds like we might want the same empty DECL_NAME handling for AGGR_INIT_EXPR_SLOT as TARGET_EXPR_SLOT. Jason
On Fri, Nov 01, 2024 at 11:23:41AM -0400, Jason Merrill wrote: > On 8/22/24 8:02 PM, Nathaniel Shead wrote: > > On Thu, Aug 22, 2024 at 02:20:14PM -0400, Patrick Palka wrote: > > > 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 <nathanieloshead@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.. > > > > > > > It's actually the opposite: the special case for empty DECL_NAME does > > kick in here and we proceed to just comparing the TARGET_EXPR_INITIAL. > > But for e.g. the 'void h(nontrivial p = nontrivial(123));' case the > > contents of the slot is an AGGR_INIT_EXPR which depends on the variable > > declared in the TARGET_EXPR_SLOT. > > > > So in this case within the default-arg-3 testcase, d_default is > > (some details elided): > > > > <target_expr 0x7ffff74ea7e0 > > type <record_type 0x7ffff76b70a8 nontrivial ...> > > side-effects tree_0 > > arg:0 <var_decl 0x7ffff76bb000 D.2886 type <record_type 0x7ffff76b70a8 nontrivial> ...> > > arg:1 <aggr_init_expr 0x7ffff76b1f40 > > type <void_type 0x7ffff7512f18 void ...> > > side-effects tree_0 > > arg:0 <integer_cst 0x7ffff76ae468 constant 5> > > arg:1 <addr_expr 0x7ffff76ba220 type <pointer_type 0x7ffff76b7b28> > > constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp >> arg:2 <var_decl 0x7ffff76bb000 D.2886> > > arg:3 <convert_expr 0x7ffff76ba240 type <pointer_type 0x7ffff76b71f8> ...> > > arg:4 <non_lvalue_expr 0x7ffff76ba260 type <integer_type 0x7ffff75125e8 int> > > constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123> ... > > > > While e_default is > > > > <target_expr 0x7ffff74ea7a8 > > type <record_type 0x7ffff76b70a8 nontrivial ...> > > side-effects tree_0 > > arg:0 <var_decl 0x7ffff74faea0 D.2860 type <record_type 0x7ffff76b70a8 nontrivial> ...> > > arg:1 <aggr_init_expr 0x7ffff76b1ac0 > > type <void_type 0x7ffff7512f18 void ...> > > side-effects tree_0 > > arg:0 <integer_cst 0x7ffff76ae468 constant 5> > > arg:1 <addr_expr 0x7ffff76ba000 type <pointer_type 0x7ffff76b7b28> > > constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp >> arg:2 <var_decl 0x7ffff74faea0 D.2860> > > arg:3 <convert_expr 0x7ffff7692e80 type <pointer_type 0x7ffff76b71f8> ...> > > arg:4 <non_lvalue_expr 0x7ffff7692e60 type <integer_type 0x7ffff75125e8 int> > > constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123> ... > > > > -- note the use of 'D.2860' vs. 'D.2886' here in arg:1 of the > > AGGR_INIT_EXPR, which causes cp_tree_equal to decide they're different. > > I imagine probably we would need to merge the VAR_DECLs here but in the > > presence of nested TARGET_EXPRs that started to get hard to figure out, > > and at some point I found I was just reimplementing a limited version of > > cp_tree_equal with an additional map so I decided to punt on it for now. > > > > Maybe a better option then would be to add a parameter to cp_tree_equal > > with an optional hash map that it could use to treat certain decls as > > 'equal' (even if they weren't), used within this special handling for > > TARGET_EXPRs, but that seems overkill for what is ultimately a pretty > > rare error case IMO. > > Sounds like we might want the same empty DECL_NAME handling for > AGGR_INIT_EXPR_SLOT as TARGET_EXPR_SLOT. > > Jason > I hadn't considered that, thanks. It looks like this neatly solves the problem. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? -- >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. * tree.cc (cp_tree_equal) <AGGR_INIT_EXPR>: Handle unification of AGGR_INIT_EXPRs with new VAR_DECL slots. gcc/testsuite/ChangeLog: * g++.dg/modules/lambda-7.h: Skip ODR-violating declaration when testing ODR deduplication. * g++.dg/modules/lambda-7_b.C: Note we're testing ODR deduplication. * 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 <nathanieloshead@gmail.com> Reviewed-by: Patrick Palka <ppalka@redhat.com> Reviewed-by: Jason Merrill <jason@redhat.com> --- gcc/cp/module.cc | 57 ++++++++++++++++++- gcc/cp/tree.cc | 33 +++++++++++ .../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 ++ gcc/testsuite/g++.dg/modules/lambda-7.h | 6 ++ gcc/testsuite/g++.dg/modules/lambda-7_b.C | 1 + 11 files changed, 206 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 2a410121321..1666249733d 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -11594,8 +11594,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 @@ -11734,7 +11732,60 @@ 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; + } + } + } + + 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)) + { + 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/cp/tree.cc b/gcc/cp/tree.cc index 911260685f1..8e566cadcaf 100644 --- a/gcc/cp/tree.cc +++ b/gcc/cp/tree.cc @@ -4042,6 +4042,39 @@ cp_tree_equal (tree t1, tree t2) TARGET_EXPR_INITIAL (t2)); } + case AGGR_INIT_EXPR: + { + int n = aggr_init_expr_nargs (t1); + if (n != aggr_init_expr_nargs (t2)) + return false; + + if (!cp_tree_equal (AGGR_INIT_EXPR_FN (t1), + AGGR_INIT_EXPR_FN (t2))) + return false; + + tree o1 = AGGR_INIT_EXPR_SLOT (t1); + tree o2 = AGGR_INIT_EXPR_SLOT (t2); + + /* Similarly to TARGET_EXPRs, if the VAR_DECL is unallocated we're + going to unify the initialization, so treat it as equivalent + to anything. */ + if (VAR_P (o1) && DECL_NAME (o1) == NULL_TREE + && !DECL_RTL_SET_P (o1)) + /*Nop*/; + else if (VAR_P (o2) && DECL_NAME (o2) == NULL_TREE + && !DECL_RTL_SET_P (o2)) + /*Nop*/; + else if (!cp_tree_equal (o1, o2)) + return false; + + for (int i = 0; i < n; ++i) + if (!cp_tree_equal (AGGR_INIT_EXPR_ARG (t1, i), + AGGR_INIT_EXPR_ARG (t2, i))) + return false; + + return true; + } + case PARM_DECL: /* For comparing uses of parameters in late-specified return types with an out-of-class definition of the function, but can also come 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..b1985c08778 --- /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" } + +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"; diff --git a/gcc/testsuite/g++.dg/modules/lambda-7.h b/gcc/testsuite/g++.dg/modules/lambda-7.h index 6f6080c1324..bfae142ed91 100644 --- a/gcc/testsuite/g++.dg/modules/lambda-7.h +++ b/gcc/testsuite/g++.dg/modules/lambda-7.h @@ -10,9 +10,15 @@ struct S { } }; +// this is an ODR violation to include this header in multiple TUs +// (the lambda is a different type in each TU, see [basic.def.odr] p15.6); +// keep the check that we can emit the lambda from a header unit, +// but skip it when checking ODR deduplication. +#ifndef CHECK_ODR_VIOLATIONS inline int d(int x, int (*f)(int) = [](int x) { return x * 5; }) { return f(x); } +#endif // unevaluated lambdas #if __cplusplus >= 202002L diff --git a/gcc/testsuite/g++.dg/modules/lambda-7_b.C b/gcc/testsuite/g++.dg/modules/lambda-7_b.C index 2d781e93067..50a3a568d1c 100644 --- a/gcc/testsuite/g++.dg/modules/lambda-7_b.C +++ b/gcc/testsuite/g++.dg/modules/lambda-7_b.C @@ -1,5 +1,6 @@ // { dg-additional-options "-fmodules-ts -fno-module-lazy -Wno-subobject-linkage" } // Test for ODR deduplication +#define CHECK_ODR_VIOLATIONS #include "lambda-7.h" import "lambda-7_a.H";
On 11/2/24 7:05 AM, Nathaniel Shead wrote: > On Fri, Nov 01, 2024 at 11:23:41AM -0400, Jason Merrill wrote: >> On 8/22/24 8:02 PM, Nathaniel Shead wrote: >>> On Thu, Aug 22, 2024 at 02:20:14PM -0400, Patrick Palka wrote: >>>> 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 <nathanieloshead@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.. >>>> >>> >>> It's actually the opposite: the special case for empty DECL_NAME does >>> kick in here and we proceed to just comparing the TARGET_EXPR_INITIAL. >>> But for e.g. the 'void h(nontrivial p = nontrivial(123));' case the >>> contents of the slot is an AGGR_INIT_EXPR which depends on the variable >>> declared in the TARGET_EXPR_SLOT. >>> >>> So in this case within the default-arg-3 testcase, d_default is >>> (some details elided): >>> >>> <target_expr 0x7ffff74ea7e0 >>> type <record_type 0x7ffff76b70a8 nontrivial ...> >>> side-effects tree_0 >>> arg:0 <var_decl 0x7ffff76bb000 D.2886 type <record_type 0x7ffff76b70a8 nontrivial> ...> >>> arg:1 <aggr_init_expr 0x7ffff76b1f40 >>> type <void_type 0x7ffff7512f18 void ...> >>> side-effects tree_0 >>> arg:0 <integer_cst 0x7ffff76ae468 constant 5> >>> arg:1 <addr_expr 0x7ffff76ba220 type <pointer_type 0x7ffff76b7b28> >>> constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp >> arg:2 <var_decl 0x7ffff76bb000 D.2886> >>> arg:3 <convert_expr 0x7ffff76ba240 type <pointer_type 0x7ffff76b71f8> ...> >>> arg:4 <non_lvalue_expr 0x7ffff76ba260 type <integer_type 0x7ffff75125e8 int> >>> constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123> ... >>> >>> While e_default is >>> >>> <target_expr 0x7ffff74ea7a8 >>> type <record_type 0x7ffff76b70a8 nontrivial ...> >>> side-effects tree_0 >>> arg:0 <var_decl 0x7ffff74faea0 D.2860 type <record_type 0x7ffff76b70a8 nontrivial> ...> >>> arg:1 <aggr_init_expr 0x7ffff76b1ac0 >>> type <void_type 0x7ffff7512f18 void ...> >>> side-effects tree_0 >>> arg:0 <integer_cst 0x7ffff76ae468 constant 5> >>> arg:1 <addr_expr 0x7ffff76ba000 type <pointer_type 0x7ffff76b7b28> >>> constant protected arg:0 <function_decl 0x7ffff76a1f00 __ct_comp >> arg:2 <var_decl 0x7ffff74faea0 D.2860> >>> arg:3 <convert_expr 0x7ffff7692e80 type <pointer_type 0x7ffff76b71f8> ...> >>> arg:4 <non_lvalue_expr 0x7ffff7692e60 type <integer_type 0x7ffff75125e8 int> >>> constant public arg:0 <integer_cst 0x7ffff76ae2d0 constant 123> ... >>> >>> -- note the use of 'D.2860' vs. 'D.2886' here in arg:1 of the >>> AGGR_INIT_EXPR, which causes cp_tree_equal to decide they're different. >>> I imagine probably we would need to merge the VAR_DECLs here but in the >>> presence of nested TARGET_EXPRs that started to get hard to figure out, >>> and at some point I found I was just reimplementing a limited version of >>> cp_tree_equal with an additional map so I decided to punt on it for now. >>> >>> Maybe a better option then would be to add a parameter to cp_tree_equal >>> with an optional hash map that it could use to treat certain decls as >>> 'equal' (even if they weren't), used within this special handling for >>> TARGET_EXPRs, but that seems overkill for what is ultimately a pretty >>> rare error case IMO. >> >> Sounds like we might want the same empty DECL_NAME handling for >> AGGR_INIT_EXPR_SLOT as TARGET_EXPR_SLOT. >> >> Jason >> > > I hadn't considered that, thanks. It looks like this neatly solves the > problem. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for > trunk? OK. > -- >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. > * tree.cc (cp_tree_equal) <AGGR_INIT_EXPR>: Handle unification > of AGGR_INIT_EXPRs with new VAR_DECL slots. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/lambda-7.h: Skip ODR-violating declaration when > testing ODR deduplication. > * g++.dg/modules/lambda-7_b.C: Note we're testing ODR > deduplication. > * 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 <nathanieloshead@gmail.com> > Reviewed-by: Patrick Palka <ppalka@redhat.com> > Reviewed-by: Jason Merrill <jason@redhat.com> > --- > gcc/cp/module.cc | 57 ++++++++++++++++++- > gcc/cp/tree.cc | 33 +++++++++++ > .../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 ++ > gcc/testsuite/g++.dg/modules/lambda-7.h | 6 ++ > gcc/testsuite/g++.dg/modules/lambda-7_b.C | 1 + > 11 files changed, 206 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 2a410121321..1666249733d 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -11594,8 +11594,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 > @@ -11734,7 +11732,60 @@ 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; > + } > + } > + } > + > + 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)) > + { > + 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/cp/tree.cc b/gcc/cp/tree.cc > index 911260685f1..8e566cadcaf 100644 > --- a/gcc/cp/tree.cc > +++ b/gcc/cp/tree.cc > @@ -4042,6 +4042,39 @@ cp_tree_equal (tree t1, tree t2) > TARGET_EXPR_INITIAL (t2)); > } > > + case AGGR_INIT_EXPR: > + { > + int n = aggr_init_expr_nargs (t1); > + if (n != aggr_init_expr_nargs (t2)) > + return false; > + > + if (!cp_tree_equal (AGGR_INIT_EXPR_FN (t1), > + AGGR_INIT_EXPR_FN (t2))) > + return false; > + > + tree o1 = AGGR_INIT_EXPR_SLOT (t1); > + tree o2 = AGGR_INIT_EXPR_SLOT (t2); > + > + /* Similarly to TARGET_EXPRs, if the VAR_DECL is unallocated we're > + going to unify the initialization, so treat it as equivalent > + to anything. */ > + if (VAR_P (o1) && DECL_NAME (o1) == NULL_TREE > + && !DECL_RTL_SET_P (o1)) > + /*Nop*/; > + else if (VAR_P (o2) && DECL_NAME (o2) == NULL_TREE > + && !DECL_RTL_SET_P (o2)) > + /*Nop*/; > + else if (!cp_tree_equal (o1, o2)) > + return false; > + > + for (int i = 0; i < n; ++i) > + if (!cp_tree_equal (AGGR_INIT_EXPR_ARG (t1, i), > + AGGR_INIT_EXPR_ARG (t2, i))) > + return false; > + > + return true; > + } > + > case PARM_DECL: > /* For comparing uses of parameters in late-specified return types > with an out-of-class definition of the function, but can also come > 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..b1985c08778 > --- /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" } > + > +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"; > diff --git a/gcc/testsuite/g++.dg/modules/lambda-7.h b/gcc/testsuite/g++.dg/modules/lambda-7.h > index 6f6080c1324..bfae142ed91 100644 > --- a/gcc/testsuite/g++.dg/modules/lambda-7.h > +++ b/gcc/testsuite/g++.dg/modules/lambda-7.h > @@ -10,9 +10,15 @@ struct S { > } > }; > > +// this is an ODR violation to include this header in multiple TUs > +// (the lambda is a different type in each TU, see [basic.def.odr] p15.6); > +// keep the check that we can emit the lambda from a header unit, > +// but skip it when checking ODR deduplication. > +#ifndef CHECK_ODR_VIOLATIONS > inline int d(int x, int (*f)(int) = [](int x) { return x * 5; }) { > return f(x); > } > +#endif > > // unevaluated lambdas > #if __cplusplus >= 202002L > diff --git a/gcc/testsuite/g++.dg/modules/lambda-7_b.C b/gcc/testsuite/g++.dg/modules/lambda-7_b.C > index 2d781e93067..50a3a568d1c 100644 > --- a/gcc/testsuite/g++.dg/modules/lambda-7_b.C > +++ b/gcc/testsuite/g++.dg/modules/lambda-7_b.C > @@ -1,5 +1,6 @@ > // { dg-additional-options "-fmodules-ts -fno-module-lazy -Wno-subobject-linkage" } > // Test for ODR deduplication > > +#define CHECK_ODR_VIOLATIONS > #include "lambda-7.h" > import "lambda-7_a.H";
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; + } + } + } + + 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. */ + && !(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";
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 <nathanieloshead@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