From: Jason Merrill <jason@redhat.com>
To: Jakub Jelinek <jakub@redhat.com>
Cc: Patrick Palka <ppalka@redhat.com>, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH, v3] c++: Fix up synthetization of defaulted comparison operators on classes with bitfields [PR102490]
Date: Thu, 30 Sep 2021 15:01:49 -0400 [thread overview]
Message-ID: <f3fe45df-7b8e-4239-7365-696e38785c4e@redhat.com> (raw)
In-Reply-To: <20210930172424.GD304296@tucnak>
On 9/30/21 13:24, Jakub Jelinek wrote:
> On Wed, Sep 29, 2021 at 03:38:45PM -0400, Jason Merrill wrote:
>> It ought to be possible to defer check_final_overrider, but it sounds
>> awkward.
>>
>> Or maybe_instantiate_noexcept could use the non-defining path in
>> build_comparison_op.
>>
>> Maybe we want a maybe_synthesize_method that actually builds the function if
>> the type is complete, or takes the non-defining path if not.
>
> So something like this?
Much like, thanks.
> spaceship-synth8.C (apparently added by me, so how valid/invalid it is is
> unclear) now doesn't ICE anymore, but without the change I've put there
> is now rejected because std::strong_ordering::less etc. aren't found.
> Previously when we were synthesizing it we did that before the FIELD_DECLs
> for bases have been added, so those weren't compared, but now they actually
> are compared.
Ah, good to have that fixed.
> After fixing the incomplete std::strong_ordering spaceship-synth8.C is now
> accepted, but I'm afraid I'm getting lost in this - clang++ rejects that
> testcase instead complaining that D has <=> operator, but has it pure virtual.
Ah, I think we need to add LOOKUP_NO_VIRTUAL to the flags variable, as
we do in do_build_copy_assign. I suppose it wouldn't hurt to add
LOOKUP_DEFAULTED as well.
One more comment in the patch below.
> And, when maybe_instantiate_noexcept tries to find out if the defaulted
> method would be implicitly deleted or not, when it does so before the class
> is complete, seems it can check whether there are errors when comparing the
> direct members of the class, but not errors about bases...
>
> 2021-09-30 Jakub Jelinek <jakub@redhat.com>
>
> PR c++/102490
> * cp-tree.h (build_comparison_op): Declare.
> * method.c (comp_info): Remove defining member.
> (comp_info::comp_info): Remove complain argument, don't initialize
> defining.
> (build_comparison_op): No longer static. Add defining argument.
> Adjust comp_info construction. Use defining instead of info.defining.
> Assert that if defining, ctype is a complete type.
> (synthesize_method, maybe_explain_implicit_delete,
> explain_implicit_non_constexpr): Adjust build_comparison_op callers.
> * class.c (check_bases_and_members): Don't call defaulted_late_check
> for sfk_comparison.
> (finish_struct_1): Call it here instead after class has been completed.
> * pt.c (maybe_instantiate_noexcept): For sfk_comparison of still
> incomplete classes, call build_comparison_op in non-defining mode
> instead of calling synthesize_method.
>
> * g++.dg/cpp2a/spaceship-synth8.C (std::strong_ordering): Provide more
> complete definition.
> (std::strong_ordering::less, std::strong_ordering::equal,
> std::strong_ordering::greater): Define.
> * g++.dg/cpp2a/spaceship-eq11.C: New test.
> * g++.dg/cpp2a/spaceship-eq12.C: New test.
>
> --- gcc/cp/cp-tree.h.jj 2021-09-18 09:44:31.728743713 +0200
> +++ gcc/cp/cp-tree.h 2021-09-30 18:39:10.416847290 +0200
> @@ -7012,6 +7012,7 @@ extern bool maybe_explain_implicit_delet
> extern void explain_implicit_non_constexpr (tree);
> extern bool deduce_inheriting_ctor (tree);
> extern bool decl_remember_implicit_trigger_p (tree);
> +extern void build_comparison_op (tree, bool, tsubst_flags_t);
> extern void synthesize_method (tree);
> extern tree lazily_declare_fn (special_function_kind,
> tree);
> --- gcc/cp/method.c.jj 2021-09-30 09:22:46.323918164 +0200
> +++ gcc/cp/method.c 2021-09-30 18:51:14.510744549 +0200
> @@ -1288,21 +1288,16 @@ struct comp_info
> {
> tree fndecl;
> location_t loc;
> - bool defining;
> bool first_time;
> bool constexp;
> bool was_constexp;
> bool noex;
>
> - comp_info (tree fndecl, tsubst_flags_t &complain)
> + comp_info (tree fndecl)
> : fndecl (fndecl)
> {
> loc = DECL_SOURCE_LOCATION (fndecl);
>
> - /* We only have tf_error set when we're called from
> - explain_invalid_constexpr_fn or maybe_explain_implicit_delete. */
> - defining = !(complain & tf_error);
> -
> first_time = DECL_MAYBE_DELETED (fndecl);
> DECL_MAYBE_DELETED (fndecl) = false;
>
> @@ -1364,12 +1359,12 @@ struct comp_info
> to use synthesize_method at the earliest opportunity and bail out if the
> function ends up being deleted. */
>
> -static void
> -build_comparison_op (tree fndecl, tsubst_flags_t complain)
> +void
> +build_comparison_op (tree fndecl, bool defining, tsubst_flags_t complain)
> {
> - comp_info info (fndecl, complain);
> + comp_info info (fndecl);
>
> - if (!info.defining && !(complain & tf_error) && !DECL_MAYBE_DELETED (fndecl))
> + if (!defining && !(complain & tf_error) && !DECL_MAYBE_DELETED (fndecl))
> return;
>
> int flags = LOOKUP_NORMAL;
> @@ -1384,6 +1379,7 @@ build_comparison_op (tree fndecl, tsubst
> lhs = convert_from_reference (lhs);
> rhs = convert_from_reference (rhs);
> tree ctype = TYPE_MAIN_VARIANT (TREE_TYPE (lhs));
> + gcc_assert (!defining || COMPLETE_TYPE_P (ctype));
>
> iloc_sentinel ils (info.loc);
>
> @@ -1399,7 +1395,7 @@ build_comparison_op (tree fndecl, tsubst
> }
>
> tree compound_stmt = NULL_TREE;
> - if (info.defining)
> + if (defining)
> compound_stmt = begin_compound_stmt (0);
> else
> ++cp_unevaluated_operand;
> @@ -1474,8 +1470,8 @@ build_comparison_op (tree fndecl, tsubst
> break;
> tree idx;
> /* [1] array, no loop needed, just add [0] ARRAY_REF.
> - Similarly if !info.defining. */
> - if (integer_zerop (maxval) || !info.defining)
> + Similarly if !defining. */
> + if (integer_zerop (maxval) || !defining)
> idx = size_zero_node;
> /* Some other array, will need runtime loop. */
> else
> @@ -1584,7 +1580,7 @@ build_comparison_op (tree fndecl, tsubst
> tree comp = comps[i];
> tree eq, retval = NULL_TREE, if_ = NULL_TREE;
> tree loop_indexes = NULL_TREE;
> - if (info.defining)
> + if (defining)
> {
> if (TREE_CODE (comp) == TREE_LIST)
> {
> @@ -1632,7 +1628,7 @@ build_comparison_op (tree fndecl, tsubst
> comp = build_static_cast (input_location, rettype, comp,
> complain);
> info.check (comp);
> - if (info.defining)
> + if (defining)
> {
> tree var = create_temporary_var (rettype);
> pushdecl (var);
> @@ -1645,7 +1641,7 @@ build_comparison_op (tree fndecl, tsubst
> }
> tree ceq = contextual_conv_bool (eq, complain);
> info.check (ceq);
> - if (info.defining)
> + if (defining)
> {
> finish_if_stmt_cond (ceq, if_);
> finish_then_clause (if_);
> @@ -1658,7 +1654,7 @@ build_comparison_op (tree fndecl, tsubst
> finish_for_stmt (TREE_VALUE (loop_index));
> }
> }
> - if (info.defining)
> + if (defining)
> {
> tree val;
> if (code == EQ_EXPR)
> @@ -1679,7 +1675,7 @@ build_comparison_op (tree fndecl, tsubst
> NULL_TREE, NULL, complain);
> comp = contextual_conv_bool (comp, complain);
> info.check (comp);
> - if (info.defining)
> + if (defining)
> {
> tree neg = build1 (TRUTH_NOT_EXPR, boolean_type_node, comp);
> finish_return_stmt (neg);
> @@ -1692,12 +1688,12 @@ build_comparison_op (tree fndecl, tsubst
> tree comp2 = build_new_op (info.loc, code, flags, comp, integer_zero_node,
> NULL_TREE, NULL, complain);
> info.check (comp2);
> - if (info.defining)
> + if (defining)
> finish_return_stmt (comp2);
> }
>
> out:
> - if (info.defining)
> + if (defining)
> finish_compound_stmt (compound_stmt);
> else
> --cp_unevaluated_operand;
> @@ -1776,7 +1772,7 @@ synthesize_method (tree fndecl)
> else if (sfk == sfk_comparison)
> {
> /* Pass tf_none so the function is just deleted if there's a problem. */
> - build_comparison_op (fndecl, tf_none);
> + build_comparison_op (fndecl, true, tf_none);
> need_body = false;
> }
>
> @@ -2747,7 +2743,7 @@ maybe_explain_implicit_delete (tree decl
> inform (DECL_SOURCE_LOCATION (decl),
> "%q#D is implicitly deleted because the default "
> "definition would be ill-formed:", decl);
> - build_comparison_op (decl, tf_warning_or_error);
> + build_comparison_op (decl, false, tf_warning_or_error);
> }
> else if (!informed)
> {
> @@ -2808,7 +2804,7 @@ explain_implicit_non_constexpr (tree dec
> if (sfk == sfk_comparison)
> {
> DECL_DECLARED_CONSTEXPR_P (decl) = true;
> - build_comparison_op (decl, tf_warning_or_error);
> + build_comparison_op (decl, false, tf_warning_or_error);
> DECL_DECLARED_CONSTEXPR_P (decl) = false;
> }
> else
> --- gcc/cp/class.c.jj 2021-09-30 09:22:46.299918499 +0200
> +++ gcc/cp/class.c 2021-09-30 18:40:52.330425343 +0200
> @@ -6133,7 +6133,8 @@ check_bases_and_members (tree t)
> no longer ill-formed, it is defined as deleted instead. */
> DECL_DELETED_FN (fn) = true;
> }
> - defaulted_late_check (fn);
> + if (special_function_p (fn) != sfk_comparison)
> + defaulted_late_check (fn);
> }
>
> if (LAMBDA_TYPE_P (t))
> @@ -7467,7 +7468,14 @@ finish_struct_1 (tree t)
> for any static member objects of the type we're working on. */
> for (x = TYPE_FIELDS (t); x; x = DECL_CHAIN (x))
> if (DECL_DECLARES_FUNCTION_P (x))
> - DECL_IN_AGGR_P (x) = false;
> + {
> + /* Synthesize constexpr defaulted comparisons. */
> + if (!DECL_ARTIFICIAL (x)
> + && DECL_DEFAULTED_IN_CLASS_P (x)
> + && special_function_p (x) == sfk_comparison)
> + defaulted_late_check (x);
> + DECL_IN_AGGR_P (x) = false;
> + }
> else if (VAR_P (x) && TREE_STATIC (x)
> && TREE_TYPE (x) != error_mark_node
> && same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (x)), t))
> --- gcc/cp/pt.c.jj 2021-09-29 10:07:41.815881043 +0200
> +++ gcc/cp/pt.c 2021-09-30 18:48:09.091331517 +0200
> @@ -25765,6 +25765,25 @@ maybe_instantiate_noexcept (tree fn, tsu
> /* We're in start_preparsed_function, keep going. */
> return true;
>
> + if (special_function_p (fn) == sfk_comparison)
> + {
> + tree lhs = DECL_ARGUMENTS (fn);
> + if (is_this_parameter (lhs))
> + lhs = cp_build_fold_indirect_ref (lhs);
> + else
> + lhs = convert_from_reference (lhs);
> + tree ctype = TYPE_MAIN_VARIANT (TREE_TYPE (lhs));
> + /* If the comparison type is still incomplete, don't synthesize the
> + method, just see if it is not implicitly deleted. */
> + if (!COMPLETE_TYPE_P (ctype))
> + {
> + push_deferring_access_checks (dk_no_deferred);
> + build_comparison_op (fn, false, tf_none);
> + pop_deferring_access_checks ();
> + return !DECL_MAYBE_DELETED (fn);
> + }
> + }
> +
> ++function_depth;
> synthesize_method (fn);
> --function_depth;
Let's factor this (from the added code to here) into a
maybe_synthesize_method in method.c. That way build_comparison_op can
stay static.
> --- gcc/testsuite/g++.dg/cpp2a/spaceship-synth8.C.jj 2020-07-28 15:39:10.012756173 +0200
> +++ gcc/testsuite/g++.dg/cpp2a/spaceship-synth8.C 2021-09-30 19:02:22.107430284 +0200
> @@ -1,7 +1,18 @@
> // PR c++/94907
> // { dg-do compile { target c++20 } }
>
> -namespace std { struct strong_ordering { }; }
> +namespace std { struct strong_ordering {
> + int _v;
> + constexpr strong_ordering (int v) :_v(v) {}
> + constexpr operator int (void) const { return _v; }
> + static const strong_ordering less;
> + static const strong_ordering equal;
> + static const strong_ordering greater;
> +};
> +constexpr strong_ordering strong_ordering::less = -1;
> +constexpr strong_ordering strong_ordering::equal = 0;
> +constexpr strong_ordering strong_ordering::greater = 1;
> +}
>
> struct E;
> struct D {
> --- gcc/testsuite/g++.dg/cpp2a/spaceship-eq11.C.jj 2021-09-30 18:34:22.897858916 +0200
> +++ gcc/testsuite/g++.dg/cpp2a/spaceship-eq11.C 2021-09-30 18:34:22.897858916 +0200
> @@ -0,0 +1,43 @@
> +// PR c++/102490
> +// { dg-do run { target c++20 } }
> +
> +struct A
> +{
> + unsigned char a : 1;
> + unsigned char b : 1;
> + constexpr bool operator== (const A &) const = default;
> +};
> +
> +struct B
> +{
> + unsigned char a : 8;
> + int : 0;
> + unsigned char b : 7;
> + constexpr bool operator== (const B &) const = default;
> +};
> +
> +struct C
> +{
> + unsigned char a : 3;
> + unsigned char b : 1;
> + constexpr bool operator== (const C &) const = default;
> +};
> +
> +void
> +foo (C &x, int y)
> +{
> + x.b = y;
> +}
> +
> +int
> +main ()
> +{
> + A a{}, b{};
> + B c{}, d{};
> + C e{}, f{};
> + a.b = 1;
> + d.b = 1;
> + foo (e, 0);
> + foo (f, 1);
> + return a == b || c == d || e == f;
> +}
> --- gcc/testsuite/g++.dg/cpp2a/spaceship-eq12.C.jj 2021-09-30 18:34:22.897858916 +0200
> +++ gcc/testsuite/g++.dg/cpp2a/spaceship-eq12.C 2021-09-30 18:34:22.897858916 +0200
> @@ -0,0 +1,5 @@
> +// PR c++/102490
> +// { dg-do run { target c++20 } }
> +// { dg-options "-O2" }
> +
> +#include "spaceship-eq11.C"
>
>
> Jakub
>
next prev parent reply other threads:[~2021-09-30 19:01 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-28 9:24 [PATCH] " Jakub Jelinek
2021-09-28 13:49 ` Patrick Palka
2021-09-28 13:53 ` Patrick Palka
2021-09-28 19:33 ` Jason Merrill
2021-09-28 20:34 ` [PATCH, v2] " Jakub Jelinek
2021-09-29 8:07 ` Jakub Jelinek
2021-09-29 18:14 ` Jason Merrill
2021-09-29 19:21 ` Jakub Jelinek
2021-09-29 19:38 ` Jason Merrill
2021-09-30 17:24 ` [PATCH, v3] " Jakub Jelinek
2021-09-30 19:01 ` Jason Merrill [this message]
2021-10-01 15:07 ` [PATCH, v4] " Jakub Jelinek
2021-10-06 2:40 ` [PATCH, v5] " Jason Merrill
2021-10-06 9:06 ` Jakub Jelinek
2021-10-06 21:13 ` Jason Merrill
2021-09-28 13:56 ` [PATCH] " Jakub Jelinek
2021-09-28 16:44 ` Patrick Palka
2021-09-28 16:49 ` Jakub Jelinek
2021-09-28 16:55 ` Jakub Jelinek
2021-09-28 17:25 ` Patrick Palka
2021-09-28 18:04 ` Jakub Jelinek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f3fe45df-7b8e-4239-7365-696e38785c4e@redhat.com \
--to=jason@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=ppalka@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).