From 4ed1dc1275bba89af92bfc7d97c21b376e4c29c3 Mon Sep 17 00:00:00 2001 From: Jason Merrill Date: Fri, 4 Dec 2020 21:48:43 -0500 Subject: [PATCH] c++: Fix defaulted <=> fallback to < and == [PR96299] I thought I had implemented P1186R3, but apparently I didn't read it closely enough to understand the point of the paper, namely that for a defaulted operator<=>, if a member type doesn't have a viable operator<=>, we will use its operator< and operator== if the defaulted operator has an specific comparison category as its return type; the compiler can't guess if it should be strong_ordering or something else, but the user can make that choice explicit. The libstdc++ test change was necessary because of the change in genericize_spaceship from op0 > op1 to op1 < op0; this should be equivalent, but isn't because of PR88173. gcc/cp/ChangeLog: PR c++/96299 * cp-tree.h (build_new_op): Add overload that omits some parms. (genericize_spaceship): Add location_t parm. * constexpr.c (cxx_eval_binary_expression): Pass it. * cp-gimplify.c (genericize_spaceship): Pass it. * method.c (genericize_spaceship): Handle class-type arguments. (build_comparison_op): Fall back to op **, tsubst_flags_t); extern bool aligned_allocation_fn_p (tree); @@ -7807,7 +7814,7 @@ extern tree merge_types (tree, tree); extern tree strip_array_domain (tree); extern tree check_return_expr (tree, bool *); extern tree spaceship_type (tree, tsubst_flags_t = tf_warning_or_error); -extern tree genericize_spaceship (tree, tree, tree); +extern tree genericize_spaceship (location_t, tree, tree, tree); extern tree cp_build_binary_op (const op_location_t &, enum tree_code, tree, tree, tsubst_flags_t); diff --git a/gcc/cp/method.c b/gcc/cp/method.c index 4de192fac00..da580a868b8 100644 --- a/gcc/cp/method.c +++ b/gcc/cp/method.c @@ -1063,43 +1063,60 @@ spaceship_type (tree optype, tsubst_flags_t complain) return lookup_comparison_category (tag, complain); } -/* Turn <=> with type TYPE and operands OP0 and OP1 into GENERIC. */ +/* Turn <=> with type TYPE and operands OP0 and OP1 into GENERIC. + This is also used by build_comparison_op for fallback to op< and op== + in a defaulted op<=>. */ tree -genericize_spaceship (tree type, tree op0, tree op1) +genericize_spaceship (location_t loc, tree type, tree op0, tree op1) { /* ??? maybe optimize based on knowledge of representation? */ comp_cat_tag tag = cat_tag_for (type); + + if (tag == cc_last && is_auto (type)) + { + /* build_comparison_op is checking to see if we want to suggest changing + the op<=> return type from auto to a specific comparison category; any + category will do for now. */ + tag = cc_strong_ordering; + type = lookup_comparison_category (tag, tf_none); + if (type == error_mark_node) + return error_mark_node; + } + gcc_checking_assert (tag < cc_last); tree r; - op0 = save_expr (op0); - op1 = save_expr (op1); + if (SCALAR_TYPE_P (TREE_TYPE (op0))) + { + op0 = save_expr (op0); + op1 = save_expr (op1); + } tree gt = lookup_comparison_result (tag, type, 1); + int flags = LOOKUP_NORMAL; + tsubst_flags_t complain = tf_none; + if (tag == cc_partial_ordering) { /* op0 == op1 ? equivalent : op0 < op1 ? less : - op0 > op1 ? greater : unordered */ + op1 < op0 ? greater : unordered */ tree uo = lookup_comparison_result (tag, type, 3); - tree comp = fold_build2 (GT_EXPR, boolean_type_node, op0, op1); - r = fold_build3 (COND_EXPR, type, comp, gt, uo); + tree comp = build_new_op (loc, LT_EXPR, flags, op1, op0, complain); + r = build_conditional_expr (loc, comp, gt, uo, complain); } else /* op0 == op1 ? equal : op0 < op1 ? less : greater */ r = gt; tree lt = lookup_comparison_result (tag, type, 2); - tree comp = fold_build2 (LT_EXPR, boolean_type_node, op0, op1); - r = fold_build3 (COND_EXPR, type, comp, lt, r); + tree comp = build_new_op (loc, LT_EXPR, flags, op0, op1, complain); + r = build_conditional_expr (loc, comp, lt, r, complain); tree eq = lookup_comparison_result (tag, type, 0); - comp = fold_build2 (EQ_EXPR, boolean_type_node, op0, op1); - r = fold_build3 (COND_EXPR, type, comp, eq, r); - - /* Wrap the whole thing in a TARGET_EXPR like build_conditional_expr_1. */ - r = get_target_expr (r); + comp = build_new_op (loc, EQ_EXPR, flags, op0, op1, complain); + r = build_conditional_expr (loc, comp, eq, r, complain); return r; } @@ -1323,7 +1340,7 @@ build_comparison_op (tree fndecl, tsubst_flags_t complain) if (!info.defining && !(complain & tf_error) && !DECL_MAYBE_DELETED (fndecl)) return; - int flags = LOOKUP_NORMAL | LOOKUP_NONVIRTUAL | LOOKUP_DEFAULTED; + int flags = LOOKUP_NORMAL; const ovl_op_info_t *op = IDENTIFIER_OVL_OP_INFO (DECL_NAME (fndecl)); tree_code code = op->tree_code; @@ -1364,6 +1381,10 @@ build_comparison_op (tree fndecl, tsubst_flags_t complain) if (code == EQ_EXPR || code == SPACESHIP_EXPR) { + comp_cat_tag retcat = cc_last; + if (code == SPACESHIP_EXPR && !FNDECL_USED_AUTO (fndecl)) + retcat = cat_tag_for (rettype); + bool bad = false; auto_vec comps; @@ -1375,13 +1396,15 @@ build_comparison_op (tree fndecl, tsubst_flags_t complain) { tree expr_type = TREE_TYPE (field); + location_t field_loc = DECL_SOURCE_LOCATION (field); + /* A defaulted comparison operator function for class C is defined as deleted if any non-static data member of C is of reference type or C has variant members. */ if (TREE_CODE (expr_type) == REFERENCE_TYPE) { if (complain & tf_error) - inform (DECL_SOURCE_LOCATION (field), "cannot default compare " + inform (field_loc, "cannot default compare " "reference member %qD", field); bad = true; continue; @@ -1390,7 +1413,7 @@ build_comparison_op (tree fndecl, tsubst_flags_t complain) && next_initializable_field (TYPE_FIELDS (expr_type))) { if (complain & tf_error) - inform (DECL_SOURCE_LOCATION (field), "cannot default compare " + inform (field_loc, "cannot default compare " "anonymous union member"); bad = true; continue; @@ -1400,25 +1423,69 @@ build_comparison_op (tree fndecl, tsubst_flags_t complain) NULL_TREE); tree rhs_mem = build3 (COMPONENT_REF, expr_type, rhs, field, NULL_TREE); - tree comp = build_new_op (info.loc, code, flags, lhs_mem, rhs_mem, - NULL_TREE, NULL, complain); + tree overload = NULL_TREE; + tree comp = build_new_op (field_loc, code, flags, lhs_mem, rhs_mem, + NULL_TREE, &overload, + retcat != cc_last ? tf_none : complain); if (comp == error_mark_node) { - bad = true; - continue; + if (overload == NULL_TREE && code == SPACESHIP_EXPR + && (retcat != cc_last || complain)) + { + tree comptype = (retcat != cc_last ? rettype + : DECL_SAVED_AUTO_RETURN_TYPE (fndecl)); + /* No viable <=>, try using op< and op==. */ + tree lteq = genericize_spaceship (field_loc, comptype, + lhs_mem, rhs_mem); + if (lteq != error_mark_node) + { + /* We found usable < and ==. */ + if (retcat != cc_last) + /* Return type is a comparison category, use them. */ + comp = lteq; + else if (complain & tf_error) + /* Return type is auto, suggest changing it. */ + inform (info.loc, "changing the return type from %qs " + "to a comparison category type will allow the " + "comparison to use %qs and %qs", "auto", + "operator<", "operator=="); + } + else if (retcat != cc_last && complain != tf_none) + /* No usable < and ==, give an error for op<=>. */ + build_new_op (field_loc, code, flags, lhs_mem, rhs_mem, + complain); + } + if (comp == error_mark_node) + { + bad = true; + continue; + } } - if (code == SPACESHIP_EXPR - && cat_tag_for (TREE_TYPE (comp)) == cc_last) + if (code != SPACESHIP_EXPR) + ; + else if (FNDECL_USED_AUTO (fndecl) + && cat_tag_for (TREE_TYPE (comp)) == cc_last) { /* The operator function is defined as deleted if ... Ri is not a comparison category type. */ if (complain & tf_error) - inform (DECL_SOURCE_LOCATION (field), + inform (field_loc, "three-way comparison of %qD has type %qT, not a " "comparison category type", field, TREE_TYPE (comp)); bad = true; continue; } + else if (!FNDECL_USED_AUTO (fndecl) + && !can_convert (rettype, TREE_TYPE (comp), complain)) + { + if (complain & tf_error) + error_at (field_loc, + "three-way comparison of %qD has type %qT, which " + "does not convert to %qT", + field, TREE_TYPE (comp), rettype); + bad = true; + continue; + } comps.safe_push (comp); } if (code == SPACESHIP_EXPR && is_auto (rettype)) diff --git a/gcc/testsuite/g++.dg/cpp2a/spaceship-p1186.C b/gcc/testsuite/g++.dg/cpp2a/spaceship-p1186.C new file mode 100644 index 00000000000..a979fcb65df --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp2a/spaceship-p1186.C @@ -0,0 +1,117 @@ +// PR c++/96299 +// P1186R3 +// { dg-do compile { target c++20 } } + +#include + +struct H { + bool operator==(H const &) const; + bool operator<(H const &) const; +}; + +// Can't deduce category from op< and op==. +struct A { + int i; + char c; + H h; // { dg-error "operator<=>" } + auto operator<=>(A const &) const = default; +}; + +// OK, explicit return type tells us how to interpret op(B const &) const = default; +}; + +struct C { + bool operator<(C const &) const; +}; + +// C's op< isn't enough, need op== as well. +struct D { + C c; // { dg-error "operator<=>" } + std::strong_ordering operator<=>(D const &) const = default; +}; + +struct E { + std::partial_ordering operator<=>(E const &) const; +}; + +// Can't use a weak op<=> to build a strong op<=>. +struct F { + E e; // { dg-error "strong_ordering" } + H h; + std::strong_ordering operator<=>(F const &) const = default; +}; + +struct G { + std::strong_ordering operator<=>(G const &) const; +}; + +// OK, uses op<=> for g and op(I const &) const = default; +}; + +template +struct J { + T t; // { dg-error "no match|ambiguous" } + auto operator<=>(J const &) const = default; +}; + +template +struct K { + T t; // { dg-error "no match" } + std::partial_ordering operator<=>(K const &) const = default; +}; + +template +struct M { + T t; // { dg-error "no match|strong_ordering" } + std::strong_ordering operator<=>(M const &) const = default; +}; + +// Test that we don't fall back to is ambiguous. +struct N { + bool operator==(N const &) const; + bool operator<(N const &) const; +}; +template +std::partial_ordering operator<=>(N const &, T&&); +template +std::partial_ordering operator<=>(T&&, N const &); + +void +foo (A &a1, A &a2, B &b1, B &b2, D& d1, D& d2, F& f1, F& f2, I& i1, I& i2) +{ + auto a = a1 < a2; // { dg-error "deleted" } + auto b = b1 < b2; + auto d = d1 < d2; // { dg-error "deleted" } + auto f = f1 < f2; // { dg-error "deleted" } + auto i = i1 < i2; + + auto j1 = J() < J(); + auto j2 = J() < J(); // { dg-error "deleted" } + auto j3 = J() < J(); // { dg-error "deleted" } + auto j4 = J() < J(); + auto j5 = J() < J(); + auto j6 = J() < J(); // { dg-error "deleted" } + + auto k1 = K() < K(); + auto k2 = K() < K(); + auto k3 = K() < K(); // { dg-error "deleted" } + auto k4 = K() < K(); + auto k5 = K() < K(); + auto k6 = K() < K(); // { dg-error "deleted" } + + auto m1 = M() < M(); + auto m2 = M() < M(); + auto m3 = M() < M(); // { dg-error "deleted" } + auto m4 = M() < M(); // { dg-error "deleted" } + auto m5 = M() < M(); + auto m6 = M() < M(); // { dg-error "deleted" } +} diff --git a/gcc/testsuite/g++.dg/cpp2a/spaceship-synth-neg2.C b/gcc/testsuite/g++.dg/cpp2a/spaceship-synth-neg2.C index 486db6cbbf9..c6acb7b5a80 100644 --- a/gcc/testsuite/g++.dg/cpp2a/spaceship-synth-neg2.C +++ b/gcc/testsuite/g++.dg/cpp2a/spaceship-synth-neg2.C @@ -14,9 +14,9 @@ bool operator<(const X&, const X&) { return true; } struct Y { int a; - X c; + X c; // { dg-error "no match" } - auto operator <=>(Y const&) const = default; // { dg-error "no match" } + auto operator <=>(Y const&) const = default; }; void f() diff --git a/libstdc++-v3/testsuite/18_support/comparisons/algorithms/partial_order.cc b/libstdc++-v3/testsuite/18_support/comparisons/algorithms/partial_order.cc index 62b379a98cb..094ac702874 100644 --- a/libstdc++-v3/testsuite/18_support/comparisons/algorithms/partial_order.cc +++ b/libstdc++-v3/testsuite/18_support/comparisons/algorithms/partial_order.cc @@ -70,7 +70,7 @@ test03() constexpr double epsilon = std::numeric_limits::epsilon(); static_assert( partial_order(denorm, smallest) == partial_ordering::less ); static_assert( partial_order(denorm, 0.0) == partial_ordering::greater ); - // FIXME: these should all use static_assert + // FIXME: these should all use static_assert. See PR88173. VERIFY( partial_order(0.0, nan) == partial_ordering::unordered ); VERIFY( partial_order(nan, nan) == partial_ordering::unordered ); VERIFY( partial_order(nan, 0.0) == partial_ordering::unordered ); @@ -81,7 +81,7 @@ test03() VERIFY( partial_order(-inf, -nan) == partial_ordering::unordered ); static_assert( partial_order(max, inf) == partial_ordering::less ); static_assert( partial_order(inf, max) == partial_ordering::greater ); - static_assert( partial_order(inf, nan) == partial_ordering::unordered ); + VERIFY( partial_order(inf, nan) == partial_ordering::unordered ); static_assert( partial_order(1.0, 1.0+epsilon) == partial_ordering::less ); } -- 2.30.2