Re: [PATCH v2] c++: make build_throw SFINAE-friendly [PR98388]
On 2/8/24 17:20, Marek Polacek wrote: On Thu, Feb 08, 2024 at 04:53:45PM -0500, Jason Merrill wrote: On 2/8/24 11:51, Marek Polacek wrote: On Thu, Feb 08, 2024 at 08:49:28AM -0500, Patrick Palka wrote: On Wed, 7 Feb 2024, Marek Polacek wrote: Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- Here the problem is that we give hard errors while substituting template parameters during overload resolution of is_throwable which has an invalid throw in decltype. The backtrace shows that fn_type_unification -> instantiate_template -> tsubst* passes complain=0 as expected, but build_throw doesn't have a complain parameter. So let's add one. Also remove a redundant local variable which I should have removed in my P2266 patch. But there's still something not quite clear to me. I claim that 'b' in the testcase should evaluate to false since the first overload ought to have been discarded. EDG 6.6 agrees, but clang++, msvc, and icx evaluate it to true. Who's right? I think it should be true since P1155, which we implement in C++20 mode and above (or rather, we implement the sequel P2266); since then we implicitly move from the function parameter. The patch looks good except that we should test this expected value. I could add #if __cplusplus >= 202002L static_assert (b, "move from the function parameter"); #else static_assert (!b, "no move from the function parameter"); #endif but that's going to fail for C++20 and above. Ah, because treat_lvalue_as_rvalue_p doesn't recognize t as being from the current scope in the trailing return type. But that shouldn't be necessary: https://eel.is/c++draft/expr.prim.id.unqual#4.2 says it's move-eligible "if the id-expression (possibly parenthesized) is the operand of a throw-expression ([expr.throw]), and names an implicitly movable entity that belongs to a scope that does not contain the compound-statement of the innermost lambda-expression, try-block, or function-try-block (if any) whose compound-statement or ctor-initializer contains the throw-expression." here there is no enclosing lambda or try, so it seems move-eligible. I wonder if this is the second half of the problem in 113789? I could comment the first static_assert and add a FIXME if that sounds good? dg-bogus would be better than commenting it out. Will you also look into fixing the treat_ bug? That can be a separate patch. Jason
Re: [PATCH v2] c++: make build_throw SFINAE-friendly [PR98388]
On Thu, Feb 08, 2024 at 04:53:45PM -0500, Jason Merrill wrote:
> On 2/8/24 11:51, Marek Polacek wrote:
> > On Thu, Feb 08, 2024 at 08:49:28AM -0500, Patrick Palka wrote:
> > > On Wed, 7 Feb 2024, Marek Polacek wrote:
> > >
> > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > > >
> > > > -- >8 --
> > > > Here the problem is that we give hard errors while substituting
> > > > template parameters during overload resolution of is_throwable
> > > > which has an invalid throw in decltype.
> > > >
> > > > The backtrace shows that fn_type_unification -> instantiate_template
> > > > -> tsubst* passes complain=0 as expected, but build_throw doesn't
> > > > have a complain parameter. So let's add one. Also remove a redundant
> > > > local variable which I should have removed in my P2266 patch.
> > > >
> > > > But there's still something not quite clear to me. I claim that 'b'
> > > > in the testcase should evaluate to false since the first overload ought
> > > > to have been discarded. EDG 6.6 agrees, but clang++, msvc, and icx
> > > > evaluate
> > > > it to true. Who's right?
>
> I think it should be true since P1155, which we implement in C++20 mode and
> above (or rather, we implement the sequel P2266); since then we implicitly
> move from the function parameter.
>
> The patch looks good except that we should test this expected value.
I could add
#if __cplusplus >= 202002L
static_assert (b, "move from the function parameter");
#else
static_assert (!b, "no move from the function parameter");
#endif
but that's going to fail for C++20 and above. I wonder if this is the
second half of the problem in 113789?
I could comment the first static_assert and add a FIXME if that sounds good?
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp0x/sfinae69.C
> > @@ -0,0 +1,16 @@
> > +// PR c++/98388
> > +// { dg-do compile { target c++11 } }
> > +
> > +struct moveonly {
> > +moveonly() = default;
> > +moveonly(moveonly&&) = default;
> > +};
> > +
> > +template
> > +constexpr auto is_throwable(T t) -> decltype(throw t, true) {
> > +return true;
> > +}
> > +template
> > +constexpr bool is_throwable(...) { return false; }
> > +
> > +constexpr bool b = is_throwable(moveonly{});
Marek
Re: [PATCH v2] c++: make build_throw SFINAE-friendly [PR98388]
On 2/8/24 11:51, Marek Polacek wrote:
On Thu, Feb 08, 2024 at 08:49:28AM -0500, Patrick Palka wrote:
On Wed, 7 Feb 2024, Marek Polacek wrote:
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
-- >8 --
Here the problem is that we give hard errors while substituting
template parameters during overload resolution of is_throwable
which has an invalid throw in decltype.
The backtrace shows that fn_type_unification -> instantiate_template
-> tsubst* passes complain=0 as expected, but build_throw doesn't
have a complain parameter. So let's add one. Also remove a redundant
local variable which I should have removed in my P2266 patch.
But there's still something not quite clear to me. I claim that 'b'
in the testcase should evaluate to false since the first overload ought
to have been discarded. EDG 6.6 agrees, but clang++, msvc, and icx evaluate
it to true. Who's right?
I think it should be true since P1155, which we implement in C++20 mode
and above (or rather, we implement the sequel P2266); since then we
implicitly move from the function parameter.
The patch looks good except that we should test this expected value.
Thanks to Patrick for notifying me of this PR. This doesn't fully fix
113789; there I think I'll have to figure our why a candidate wasn't
discarded from the overload set.
gcc/cp/ChangeLog:
* coroutines.cc (coro_rewrite_function_body): Pass tf_warning_or_error
to build_throw.
(morph_fn_to_coro): Likewise.
* cp-tree.h (build_throw): Adjust.
* except.cc (expand_end_catch_block): Pass tf_warning_or_error to
build_throw.
(build_throw): Add a tsubst_flags_t parameter. Use it. Remove
redundant variable. Guard an inform call.
* parser.cc (cp_parser_throw_expression): Pass tf_warning_or_error
to build_throw.
* pt.cc (tsubst_expr) : Pass complain to build_throw.
libcc1/ChangeLog:
* libcp1plugin.cc (plugin_build_unary_expr): Pass tf_error to
build_throw.
gcc/testsuite/ChangeLog:
* g++.dg/cpp0x/sfinae69.C: New test.
---
gcc/cp/coroutines.cc | 4 ++--
gcc/cp/cp-tree.h | 3 ++-
gcc/cp/except.cc | 31 +++
gcc/cp/parser.cc | 2 +-
gcc/cp/pt.cc | 2 +-
gcc/testsuite/g++.dg/cpp0x/sfinae69.C | 16 ++
libcc1/libcp1plugin.cc| 4 ++--
7 files changed, 37 insertions(+), 25 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp0x/sfinae69.C
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 3194c911e8c..9b037edbd14 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -4246,7 +4246,7 @@ coro_rewrite_function_body (location_t fn_start, tree
fnbody, tree orig,
boolean_type_node, i_a_r_c);
finish_if_stmt_cond (not_iarc, not_iarc_if);
/* If the initial await resume called value is false, rethrow... */
- tree rethrow = build_throw (fn_start, NULL_TREE);
+ tree rethrow = build_throw (fn_start, NULL_TREE, tf_warning_or_error);
suppress_warning (rethrow);
finish_expr_stmt (rethrow);
finish_then_clause (not_iarc_if);
@@ -5151,7 +5151,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree
*destroyer)
tree del_coro_fr = coro_get_frame_dtor (coro_fp, orig, frame_size,
promise_type, fn_start);
finish_expr_stmt (del_coro_fr);
- tree rethrow = build_throw (fn_start, NULL_TREE);
+ tree rethrow = build_throw (fn_start, NULL_TREE, tf_warning_or_error);
suppress_warning (rethrow);
finish_expr_stmt (rethrow);
finish_handler (handler);
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 969c7239c97..334c11396c2 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7185,7 +7185,8 @@ extern void init_exception_processing (void);
extern tree expand_start_catch_block (tree);
extern void expand_end_catch_block(void);
extern tree build_exc_ptr (void);
-extern tree build_throw(location_t, tree);
+extern tree build_throw(location_t, tree,
+tsubst_flags_t);
extern int nothrow_libfn_p(const_tree);
extern void check_handlers(tree);
extern tree finish_noexcept_expr (tree, tsubst_flags_t);
diff --git a/gcc/cp/except.cc b/gcc/cp/except.cc
index d17a57d3fbc..ed704b6c28a 100644
--- a/gcc/cp/except.cc
+++ b/gcc/cp/except.cc
@@ -486,7 +486,8 @@ expand_end_catch_block (void)
|| DECL_DESTRUCTOR_P (current_function_decl))
&& !in_nested_catch ())
{
- tree rethrow = build_throw (input_location, NULL_TREE);
+ tree rethrow = build_throw (input_location, NULL_TREE,
+
[PATCH v2] c++: make build_throw SFINAE-friendly [PR98388]
On Thu, Feb 08, 2024 at 08:49:28AM -0500, Patrick Palka wrote:
> On Wed, 7 Feb 2024, Marek Polacek wrote:
>
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> >
> > -- >8 --
> > Here the problem is that we give hard errors while substituting
> > template parameters during overload resolution of is_throwable
> > which has an invalid throw in decltype.
> >
> > The backtrace shows that fn_type_unification -> instantiate_template
> > -> tsubst* passes complain=0 as expected, but build_throw doesn't
> > have a complain parameter. So let's add one. Also remove a redundant
> > local variable which I should have removed in my P2266 patch.
> >
> > But there's still something not quite clear to me. I claim that 'b'
> > in the testcase should evaluate to false since the first overload ought
> > to have been discarded. EDG 6.6 agrees, but clang++, msvc, and icx evaluate
> > it to true. Who's right?
> >
> > Thanks to Patrick for notifying me of this PR. This doesn't fully fix
> > 113789; there I think I'll have to figure our why a candidate wasn't
> > discarded from the overload set.
> >
> > gcc/cp/ChangeLog:
> >
> > * coroutines.cc (coro_rewrite_function_body): Pass tf_warning_or_error
> > to build_throw.
> > (morph_fn_to_coro): Likewise.
> > * cp-tree.h (build_throw): Adjust.
> > * except.cc (expand_end_catch_block): Pass tf_warning_or_error to
> > build_throw.
> > (build_throw): Add a tsubst_flags_t parameter. Use it. Remove
> > redundant variable. Guard an inform call.
> > * parser.cc (cp_parser_throw_expression): Pass tf_warning_or_error
> > to build_throw.
> > * pt.cc (tsubst_expr) : Pass complain to build_throw.
> >
> > libcc1/ChangeLog:
> >
> > * libcp1plugin.cc (plugin_build_unary_expr): Pass tf_error to
> > build_throw.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/cpp0x/sfinae69.C: New test.
> > ---
> > gcc/cp/coroutines.cc | 4 ++--
> > gcc/cp/cp-tree.h | 3 ++-
> > gcc/cp/except.cc | 31 +++
> > gcc/cp/parser.cc | 2 +-
> > gcc/cp/pt.cc | 2 +-
> > gcc/testsuite/g++.dg/cpp0x/sfinae69.C | 16 ++
> > libcc1/libcp1plugin.cc| 4 ++--
> > 7 files changed, 37 insertions(+), 25 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/cpp0x/sfinae69.C
> >
> > diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> > index 3194c911e8c..9b037edbd14 100644
> > --- a/gcc/cp/coroutines.cc
> > +++ b/gcc/cp/coroutines.cc
> > @@ -4246,7 +4246,7 @@ coro_rewrite_function_body (location_t fn_start, tree
> > fnbody, tree orig,
> > boolean_type_node, i_a_r_c);
> >finish_if_stmt_cond (not_iarc, not_iarc_if);
> >/* If the initial await resume called value is false, rethrow... */
> > - tree rethrow = build_throw (fn_start, NULL_TREE);
> > + tree rethrow = build_throw (fn_start, NULL_TREE,
> > tf_warning_or_error);
> >suppress_warning (rethrow);
> >finish_expr_stmt (rethrow);
> >finish_then_clause (not_iarc_if);
> > @@ -5151,7 +5151,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree
> > *destroyer)
> >tree del_coro_fr = coro_get_frame_dtor (coro_fp, orig, frame_size,
> > promise_type, fn_start);
> >finish_expr_stmt (del_coro_fr);
> > - tree rethrow = build_throw (fn_start, NULL_TREE);
> > + tree rethrow = build_throw (fn_start, NULL_TREE,
> > tf_warning_or_error);
> >suppress_warning (rethrow);
> >finish_expr_stmt (rethrow);
> >finish_handler (handler);
> > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> > index 969c7239c97..334c11396c2 100644
> > --- a/gcc/cp/cp-tree.h
> > +++ b/gcc/cp/cp-tree.h
> > @@ -7185,7 +7185,8 @@ extern void init_exception_processing (void);
> > extern tree expand_start_catch_block (tree);
> > extern void expand_end_catch_block (void);
> > extern tree build_exc_ptr (void);
> > -extern tree build_throw(location_t, tree);
> > +extern tree build_throw(location_t, tree,
> > +tsubst_flags_t);
> > extern int nothrow_libfn_p (const_tree);
> > extern void check_handlers (tree);
> > extern tree finish_noexcept_expr (tree, tsubst_flags_t);
> > diff --git a/gcc/cp/except.cc b/gcc/cp/except.cc
> > index d17a57d3fbc..ed704b6c28a 100644
> > --- a/gcc/cp/except.cc
> > +++ b/gcc/cp/except.cc
> > @@ -486,7 +486,8 @@ expand_end_catch_block (void)
> > || DECL_DESTRUCTOR_P (current_function_decl))
> >&& !in_nested_catch ())
> > {
> > - tree rethrow = build_throw (input_location, NULL_TREE);
> > + tree rethrow = build_throw (input_location, NULL_TREE,
> > +
