Re: [PATCH v2] c++/modules: Support lambdas attached to more places in modules [PR111710]
On Fri, 16 Feb 2024, Nathaniel Shead wrote:
> On Tue, Feb 13, 2024 at 07:52:01PM -0500, Jason Merrill wrote:
> > On 2/10/24 17:57, Nathaniel Shead wrote:
> > > The fix for PR107398 weakened the restrictions that lambdas must belong
> > > to namespace scope. However this was not sufficient: we also need to
> > > allow lambdas keyed to FIELD_DECLs or PARM_DECLs.
> >
> > I wonder about keying such lambdas to the class and function, respectively,
> > rather than specifically to the field or parameter, but I suppose it doesn't
> > matter.
>
> I did some more testing and realised my testcase didn't properly
> exercise whether I'd properly deduplicated or not, and an improved
> testcase proved that actually keying to the field rather than the class
> did cause issues. (Parameter vs. function doesn't seem to have mattered
> however.)
>
> Here's an updated patch that fixes this, and includes the changes for
> lambdas in base classes that I'd had as a separate patch earlier. I've
> also added some concepts testcases just in case.
>
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>
> -- >8 --
>
> The fix for PR107398 weakened the restrictions that lambdas must belong
> to namespace scope. However this was not sufficient: we also need to
> allow lambdas attached to FIELD_DECLs, PARM_DECLs, and TYPE_DECLs.
>
> For field decls we key the lambda to its class rather than the field
> itself. This avoids some errors with deduplicating fields.
>
> Additionally, by [basic.link] p15.2 a lambda defined anywhere in a
> class-specifier should not be TU-local, which includes base-class
> declarations, so ensure that lambdas declared there are keyed
> appropriately as well.
>
> Because this now requires 'DECL_MODULE_KEYED_DECLS_P' to be checked on a
> fairly large number of different kinds of DECLs, and that in general
> it's safe to just get 'false' as a result of a check on an unexpected
> DECL type, this patch also removes the tree checking from the accessor.
>
> Finally, to handle deduplicating templated lambda fields, we need to
> ensure that we can determine that two lambdas from different field decls
> match. The modules code does not attempt to deduplicate expression
> nodes, which causes issues as the LAMBDA_EXPRs are then considered to be
> different. However, rather than checking the LAMBDA_EXPR directly we can
> instead check its type: the generated RECORD_TYPE for a LAMBDA_EXPR must
> also be unique, and /is/ deduplicated on import, so we can just check
> for that instead.
We probably should be deduplicating LAMBDA_EXPR on stream-in, perhaps
something like
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index e8eabb1f6f9..1b2ba2e0fa8 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -9183,6 +9183,13 @@ trees_in::tree_value ()
return NULL_TREE;
}
+ if (TREE_CODE (t) == LAMBDA_EXPR
+ && CLASSTYPE_LAMBDA_EXPR (TREE_TYPE (t)))
+{
+ existing = CLASSTYPE_LAMBDA_EXPR (TREE_TYPE (t));
+ back_refs[~tag] = existing;
+}
+
dump (dumper::TREE) && dump ("Read tree:%d %C:%N", tag, TREE_CODE (t), t);
if (TREE_CODE (existing) == INTEGER_CST && !TREE_OVERFLOW (existing))
would suffice? If not we probably need to take inspiration from the
TREE_BINFO streaming, and handle LAMBDA_EXPR similarly..
>
> PR c++/111710
>
> gcc/cp/ChangeLog:
>
> * cp-tree.h (DECL_MODULE_KEYED_DECLS_P): Remove tree checking.
> (struct lang_decl_base): Update comments and fix whitespace.
> * module.cc (trees_out::lang_decl_bools): Always write
> module_keyed_decls_p flag...
> (trees_in::lang_decl_bools): ...and always read it.
> (trees_out::decl_value): Handle all kinds of keyed decls.
> (trees_in::decl_value): Likewise.
> (maybe_key_decl): Also support lambdas attached to fields,
> parameters, and types. Key lambdas attached to fields to their
> class.
> (trees_out::get_merge_kind): Likewise.
> (trees_out::key_mergeable): Likewise.
> (trees_in::key_mergeable): Support keyed decls in a TYPE_DECL
> container.
> * parser.cc (cp_parser_class_head): Start a lambda scope when
> parsing base classes.
> * tree.cc (cp_tree_equal): Check equality of the types of
> LAMBDA_EXPRs instead of the exprs themselves.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/modules/lambda-7.h: New test.
> * g++.dg/modules/lambda-7_a.H: New test.
> * g++.dg/modules/lambda-7_b.C: New test.
> * g++.dg/modules/lambda-7_c.C: New test.
>
> Signed-off-by: Nathaniel Shead
> ---
> gcc/cp/cp-tree.h | 26 +++
> gcc/cp/module.cc | 94 +--
> gcc/cp/parser.cc | 10 ++-
> gcc/cp/tree.cc| 4 +-
> gcc/testsuite/g++.dg/modules/lambda-7.h | 42 ++
> gcc/testsuite/g++.dg/modules/lambda-7_a.H | 4 +
> gcc/testsuite/g++.dg/modules/lambda-7_b.C | 5
[PATCH v2] c++/modules: Support lambdas attached to more places in modules [PR111710]
On Tue, Feb 13, 2024 at 07:52:01PM -0500, Jason Merrill wrote: > On 2/10/24 17:57, Nathaniel Shead wrote: > > The fix for PR107398 weakened the restrictions that lambdas must belong > > to namespace scope. However this was not sufficient: we also need to > > allow lambdas keyed to FIELD_DECLs or PARM_DECLs. > > I wonder about keying such lambdas to the class and function, respectively, > rather than specifically to the field or parameter, but I suppose it doesn't > matter. I did some more testing and realised my testcase didn't properly exercise whether I'd properly deduplicated or not, and an improved testcase proved that actually keying to the field rather than the class did cause issues. (Parameter vs. function doesn't seem to have mattered however.) Here's an updated patch that fixes this, and includes the changes for lambdas in base classes that I'd had as a separate patch earlier. I've also added some concepts testcases just in case. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? -- >8 -- The fix for PR107398 weakened the restrictions that lambdas must belong to namespace scope. However this was not sufficient: we also need to allow lambdas attached to FIELD_DECLs, PARM_DECLs, and TYPE_DECLs. For field decls we key the lambda to its class rather than the field itself. This avoids some errors with deduplicating fields. Additionally, by [basic.link] p15.2 a lambda defined anywhere in a class-specifier should not be TU-local, which includes base-class declarations, so ensure that lambdas declared there are keyed appropriately as well. Because this now requires 'DECL_MODULE_KEYED_DECLS_P' to be checked on a fairly large number of different kinds of DECLs, and that in general it's safe to just get 'false' as a result of a check on an unexpected DECL type, this patch also removes the tree checking from the accessor. Finally, to handle deduplicating templated lambda fields, we need to ensure that we can determine that two lambdas from different field decls match. The modules code does not attempt to deduplicate expression nodes, which causes issues as the LAMBDA_EXPRs are then considered to be different. However, rather than checking the LAMBDA_EXPR directly we can instead check its type: the generated RECORD_TYPE for a LAMBDA_EXPR must also be unique, and /is/ deduplicated on import, so we can just check for that instead. PR c++/111710 gcc/cp/ChangeLog: * cp-tree.h (DECL_MODULE_KEYED_DECLS_P): Remove tree checking. (struct lang_decl_base): Update comments and fix whitespace. * module.cc (trees_out::lang_decl_bools): Always write module_keyed_decls_p flag... (trees_in::lang_decl_bools): ...and always read it. (trees_out::decl_value): Handle all kinds of keyed decls. (trees_in::decl_value): Likewise. (maybe_key_decl): Also support lambdas attached to fields, parameters, and types. Key lambdas attached to fields to their class. (trees_out::get_merge_kind): Likewise. (trees_out::key_mergeable): Likewise. (trees_in::key_mergeable): Support keyed decls in a TYPE_DECL container. * parser.cc (cp_parser_class_head): Start a lambda scope when parsing base classes. * tree.cc (cp_tree_equal): Check equality of the types of LAMBDA_EXPRs instead of the exprs themselves. gcc/testsuite/ChangeLog: * g++.dg/modules/lambda-7.h: New test. * g++.dg/modules/lambda-7_a.H: New test. * g++.dg/modules/lambda-7_b.C: New test. * g++.dg/modules/lambda-7_c.C: New test. Signed-off-by: Nathaniel Shead --- gcc/cp/cp-tree.h | 26 +++ gcc/cp/module.cc | 94 +-- gcc/cp/parser.cc | 10 ++- gcc/cp/tree.cc| 4 +- gcc/testsuite/g++.dg/modules/lambda-7.h | 42 ++ gcc/testsuite/g++.dg/modules/lambda-7_a.H | 4 + gcc/testsuite/g++.dg/modules/lambda-7_b.C | 5 ++ gcc/testsuite/g++.dg/modules/lambda-7_c.C | 41 ++ 8 files changed, 169 insertions(+), 57 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/lambda-7.h create mode 100644 gcc/testsuite/g++.dg/modules/lambda-7_a.H create mode 100644 gcc/testsuite/g++.dg/modules/lambda-7_b.C create mode 100644 gcc/testsuite/g++.dg/modules/lambda-7_c.C diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 334c11396c2..04c3aa6cd91 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -1773,9 +1773,8 @@ check_constraint_info (tree t) (DECL_LANG_SPECIFIC (DECL_MODULE_CHECK (NODE))->u.base.module_entity_p) /* DECL that has attached decls for ODR-relatedness. */ -#define DECL_MODULE_KEYED_DECLS_P(NODE)\ - (DECL_LANG_SPECIFIC (TREE_CHECK2(NODE,FUNCTION_DECL,VAR_DECL))\ - ->u.base.module_keyed_decls_p) +#define DECL_MODULE_KEYED_DECLS_P(NODE) \ + (DECL_LANG_SPECIFIC (DECL_MODULE_CHECK
