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 <nathanielosh...@gmail.com>
> > ---
> > gcc/cp/module.cc | 62 ++++++++++++++++++-
> > .../g++.dg/modules/default-arg-1_a.H | 17 +++++
> > .../g++.dg/modules/default-arg-1_b.C | 26 ++++++++
> > .../g++.dg/modules/default-arg-2_a.H | 17 +++++
> > .../g++.dg/modules/default-arg-2_b.C | 28 +++++++++
> > gcc/testsuite/g++.dg/modules/default-arg-3.h | 13 ++++
> > .../g++.dg/modules/default-arg-3_a.H | 5 ++
> > .../g++.dg/modules/default-arg-3_b.C | 6 ++
> > 8 files changed, 171 insertions(+), 3 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-1_a.H
> > create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-1_b.C
> > create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-2_a.H
> > create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-2_b.C
> > create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-3.h
> > create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-3_a.H
> > create mode 100644 gcc/testsuite/g++.dg/modules/default-arg-3_b.C
> >
> > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> > index f4d137b13a1..87f34bac578 100644
> > --- a/gcc/cp/module.cc
> > +++ b/gcc/cp/module.cc
> > @@ -11551,8 +11551,6 @@ trees_in::is_matching_decl (tree existing, tree
> > decl, bool is_typedef)
> >
> > if (!same_type_p (TREE_VALUE (d_args), TREE_VALUE (e_args)))
> > goto mismatch;
> > -
> > - // FIXME: Check default values
> > }
> >
> > /* If EXISTING has an undeduced or uninstantiated exception
> > @@ -11690,7 +11688,65 @@ trees_in::is_matching_decl (tree existing, tree
> > decl, bool is_typedef)
> > if (!DECL_EXTERNAL (d_inner))
> > DECL_EXTERNAL (e_inner) = false;
> >
> > - // FIXME: Check default tmpl and fn parms here
> > + if (TREE_CODE (decl) == TEMPLATE_DECL)
> > + {
> > + /* Merge default template arguments. */
> > + tree d_parms = DECL_INNERMOST_TEMPLATE_PARMS (decl);
> > + tree e_parms = DECL_INNERMOST_TEMPLATE_PARMS (existing);
> > + gcc_checking_assert (TREE_VEC_LENGTH (d_parms)
> > + == TREE_VEC_LENGTH (e_parms));
> > + for (int i = 0; i < TREE_VEC_LENGTH (d_parms); ++i)
> > + {
> > + tree d_default = TREE_PURPOSE (TREE_VEC_ELT (d_parms, i));
> > + tree& e_default = TREE_PURPOSE (TREE_VEC_ELT (e_parms, i));
> > + if (e_default == NULL_TREE)
> > + e_default = d_default;
> > + else if (d_default != NULL_TREE
> > + && !cp_tree_equal (d_default, e_default))
> > + {
> > + auto_diagnostic_group d;
> > + tree d_parm = TREE_VALUE (TREE_VEC_ELT (d_parms, i));
> > + tree e_parm = TREE_VALUE (TREE_VEC_ELT (e_parms, i));
> > + error_at (DECL_SOURCE_LOCATION (d_parm),
> > + "conflicting default argument for %#qD", d_parm);
> > + inform (DECL_SOURCE_LOCATION (e_parm),
> > + "existing default declared here");
> > + return false;
> > + }
> > + }
> > + }
>
> FWIW for ordinary textual redeclarations we merge default template
> arguments via merge_default_template_args. But the semantics are
> slightly different on stream in because we want to allow a default
> argument redefinition as long as it's cp_tree_equal, so I don't think
> we can or want to use merge_default_template_args here.
>
> > +
> > + if (TREE_CODE (d_inner) == FUNCTION_DECL)
> > + {
> > + /* Merge default function arguments. */
> > + tree d_parm = FUNCTION_FIRST_USER_PARMTYPE (d_inner);
> > + tree e_parm = FUNCTION_FIRST_USER_PARMTYPE (e_inner);
> > + int i = 0;
> > + for (; d_parm && d_parm != void_list_node;
> > + d_parm = TREE_CHAIN (d_parm), e_parm = TREE_CHAIN (e_parm), ++i)
> > + {
> > + tree d_default = TREE_PURPOSE (d_parm);
> > + tree& e_default = TREE_PURPOSE (e_parm);
> > + if (e_default == NULL_TREE)
> > + e_default = d_default;
> > + else if (d_default != NULL_TREE
> > + && !cp_tree_equal (d_default, e_default)
> > + /* FIXME: We need to remap the target to match the existing
> > + parms so that cp_tree_equal works correctly, as with
> > + break_out_target_exprs. */
>
> Is the problem that cp_tree_equal expects TARGET_EXPR_SLOT to be
> identical (despite the probably bitrotted special case for empty
> DECL_NAME)? Maybe it'd make sense to always only compare
> TARGET_EXPR_INITIAL, not TARGET_EXPR_SLOT..
>
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
> >
> >
>
