[v2] c++: make build_throw SFINAE-friendly [PR98388]
Checks
Commit Message
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) <case THROW_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,
> > + tf_warning_or_error);
> > /* Disable all warnings for the generated rethrow statement. */
> > suppress_warning (rethrow);
> > finish_expr_stmt (rethrow);
> > @@ -607,7 +608,7 @@ wrap_cleanups_r (tree *tp, int *walk_subtrees, void * /*data*/)
> > /* Build a throw expression. */
> >
> > tree
> > -build_throw (location_t loc, tree exp)
> > +build_throw (location_t loc, tree exp, tsubst_flags_t complain)
> > {
> > if (exp == error_mark_node)
> > return exp;
>
> The "throwing NULL, which has integral, not pointer type" warning near
> the top of build_throw should probably be guarded by tf_warning now.
Ah, I missed that. Fixed.
> > @@ -642,8 +643,6 @@ build_throw (location_t loc, tree exp)
> > tree object, ptr;
> > tree allocate_expr;
> >
> > - tsubst_flags_t complain = tf_warning_or_error;
> > -
> > /* The CLEANUP_TYPE is the internal type of a destructor. */
> > if (!cleanup_type)
> > {
> > @@ -714,7 +713,6 @@ build_throw (location_t loc, tree exp)
> > if (CLASS_TYPE_P (temp_type))
> > {
> > int flags = LOOKUP_NORMAL | LOOKUP_ONLYCONVERTING;
> > - bool converted = false;
> > location_t exp_loc = cp_expr_loc_or_loc (exp, loc);
> >
> > /* Under C++0x [12.8/16 class.copy], a thrown lvalue is sometimes
> > @@ -727,23 +725,20 @@ build_throw (location_t loc, tree exp)
> > exp = moved;
> >
> > /* Call the copy constructor. */
> > - if (!converted)
> > - {
> > - releasing_vec exp_vec (make_tree_vector_single (exp));
> > - exp = (build_special_member_call
> > - (object, complete_ctor_identifier, &exp_vec,
> > - TREE_TYPE (object), flags, tf_warning_or_error));
> > - }
> > -
> > + releasing_vec exp_vec (make_tree_vector_single (exp));
> > + exp = build_special_member_call (object, complete_ctor_identifier,
> > + &exp_vec, TREE_TYPE (object), flags,
> > + complain);
> > if (exp == error_mark_node)
> > {
> > - inform (exp_loc, " in thrown expression");
> > + if (complain & tf_warning)
>
> tf_error instead of tf_warning for this note?
Not sure it makes a difference in practice, but I've changed it. Thanks,
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) <case THROW_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 | 33 ++++++++++++---------------
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, 38 insertions(+), 26 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp0x/sfinae69.C
base-commit: 5fb204aaf34b68c427f5b2bfb933fed72fe3eafb
Comments
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) <case THROW_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,
>>> + tf_warning_or_error);
>>> /* Disable all warnings for the generated rethrow statement. */
>>> suppress_warning (rethrow);
>>> finish_expr_stmt (rethrow);
>>> @@ -607,7 +608,7 @@ wrap_cleanups_r (tree *tp, int *walk_subtrees, void * /*data*/)
>>> /* Build a throw expression. */
>>>
>>> tree
>>> -build_throw (location_t loc, tree exp)
>>> +build_throw (location_t loc, tree exp, tsubst_flags_t complain)
>>> {
>>> if (exp == error_mark_node)
>>> return exp;
>>
>> The "throwing NULL, which has integral, not pointer type" warning near
>> the top of build_throw should probably be guarded by tf_warning now.
>
> Ah, I missed that. Fixed.
>
>>> @@ -642,8 +643,6 @@ build_throw (location_t loc, tree exp)
>>> tree object, ptr;
>>> tree allocate_expr;
>>>
>>> - tsubst_flags_t complain = tf_warning_or_error;
>>> -
>>> /* The CLEANUP_TYPE is the internal type of a destructor. */
>>> if (!cleanup_type)
>>> {
>>> @@ -714,7 +713,6 @@ build_throw (location_t loc, tree exp)
>>> if (CLASS_TYPE_P (temp_type))
>>> {
>>> int flags = LOOKUP_NORMAL | LOOKUP_ONLYCONVERTING;
>>> - bool converted = false;
>>> location_t exp_loc = cp_expr_loc_or_loc (exp, loc);
>>>
>>> /* Under C++0x [12.8/16 class.copy], a thrown lvalue is sometimes
>>> @@ -727,23 +725,20 @@ build_throw (location_t loc, tree exp)
>>> exp = moved;
>>>
>>> /* Call the copy constructor. */
>>> - if (!converted)
>>> - {
>>> - releasing_vec exp_vec (make_tree_vector_single (exp));
>>> - exp = (build_special_member_call
>>> - (object, complete_ctor_identifier, &exp_vec,
>>> - TREE_TYPE (object), flags, tf_warning_or_error));
>>> - }
>>> -
>>> + releasing_vec exp_vec (make_tree_vector_single (exp));
>>> + exp = build_special_member_call (object, complete_ctor_identifier,
>>> + &exp_vec, TREE_TYPE (object), flags,
>>> + complain);
>>> if (exp == error_mark_node)
>>> {
>>> - inform (exp_loc, " in thrown expression");
>>> + if (complain & tf_warning)
>>
>> tf_error instead of tf_warning for this note?
>
> Not sure it makes a difference in practice, but I've changed it. Thanks,
>
> 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) <case THROW_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 | 33 ++++++++++++---------------
> 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, 38 insertions(+), 26 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..ea3d6f57396 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,
> + tf_warning_or_error);
> /* Disable all warnings for the generated rethrow statement. */
> suppress_warning (rethrow);
> finish_expr_stmt (rethrow);
> @@ -607,7 +608,7 @@ wrap_cleanups_r (tree *tp, int *walk_subtrees, void * /*data*/)
> /* Build a throw expression. */
>
> tree
> -build_throw (location_t loc, tree exp)
> +build_throw (location_t loc, tree exp, tsubst_flags_t complain)
> {
> if (exp == error_mark_node)
> return exp;
> @@ -621,7 +622,7 @@ build_throw (location_t loc, tree exp)
> return exp;
> }
>
> - if (exp && null_node_p (exp))
> + if (exp && null_node_p (exp) && (complain & tf_warning))
> warning_at (loc, 0,
> "throwing NULL, which has integral, not pointer type");
>
> @@ -642,8 +643,6 @@ build_throw (location_t loc, tree exp)
> tree object, ptr;
> tree allocate_expr;
>
> - tsubst_flags_t complain = tf_warning_or_error;
> -
> /* The CLEANUP_TYPE is the internal type of a destructor. */
> if (!cleanup_type)
> {
> @@ -714,7 +713,6 @@ build_throw (location_t loc, tree exp)
> if (CLASS_TYPE_P (temp_type))
> {
> int flags = LOOKUP_NORMAL | LOOKUP_ONLYCONVERTING;
> - bool converted = false;
> location_t exp_loc = cp_expr_loc_or_loc (exp, loc);
>
> /* Under C++0x [12.8/16 class.copy], a thrown lvalue is sometimes
> @@ -727,23 +725,20 @@ build_throw (location_t loc, tree exp)
> exp = moved;
>
> /* Call the copy constructor. */
> - if (!converted)
> - {
> - releasing_vec exp_vec (make_tree_vector_single (exp));
> - exp = (build_special_member_call
> - (object, complete_ctor_identifier, &exp_vec,
> - TREE_TYPE (object), flags, tf_warning_or_error));
> - }
> -
> + releasing_vec exp_vec (make_tree_vector_single (exp));
> + exp = build_special_member_call (object, complete_ctor_identifier,
> + &exp_vec, TREE_TYPE (object), flags,
> + complain);
> if (exp == error_mark_node)
> {
> - inform (exp_loc, " in thrown expression");
> + if (complain & tf_error)
> + inform (exp_loc, " in thrown expression");
> return error_mark_node;
> }
> }
> else
> {
> - tree tmp = decay_conversion (exp, tf_warning_or_error);
> + tree tmp = decay_conversion (exp, complain);
> if (tmp == error_mark_node)
> return error_mark_node;
> exp = cp_build_init_expr (object, tmp);
> @@ -768,7 +763,7 @@ build_throw (location_t loc, tree exp)
> tree binfo = TYPE_BINFO (TREE_TYPE (object));
> tree dtor_fn = lookup_fnfields (binfo,
> complete_dtor_identifier, 0,
> - tf_warning_or_error);
> + complain);
> dtor_fn = BASELINK_FUNCTIONS (dtor_fn);
> if (!mark_used (dtor_fn)
> || !perform_or_defer_access_check (binfo, dtor_fn,
> @@ -785,7 +780,7 @@ build_throw (location_t loc, tree exp)
> cleanup = build_int_cst (cleanup_type, 0);
>
> /* ??? Indicate that this function call throws throw_type. */
> - tree tmp = cp_build_function_call_nary (throw_fn, tf_warning_or_error,
> + tree tmp = cp_build_function_call_nary (throw_fn, complain,
> ptr, throw_type, cleanup,
> NULL_TREE);
>
> @@ -807,7 +802,7 @@ build_throw (location_t loc, tree exp)
>
> /* ??? Indicate that this function call allows exceptions of the type
> of the enclosing catch block (if known). */
> - exp = cp_build_function_call_vec (rethrow_fn, NULL, tf_warning_or_error);
> + exp = cp_build_function_call_vec (rethrow_fn, NULL, complain);
> }
>
> exp = build1_loc (loc, THROW_EXPR, void_type_node, exp);
> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> index c4292c49ce2..09ecfa23b5d 100644
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -29310,7 +29310,7 @@ cp_parser_throw_expression (cp_parser* parser)
> the end at the end of the final token we consumed. */
> location_t combined_loc = make_location (start_loc, start_loc,
> parser->lexer);
> - expression = build_throw (combined_loc, expression);
> + expression = build_throw (combined_loc, expression, tf_warning_or_error);
>
> return expression;
> }
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index f9abb21a9a2..9c225c095c8 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -21180,7 +21180,7 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl)
>
> case THROW_EXPR:
> RETURN (build_throw
> - (input_location, RECUR (TREE_OPERAND (t, 0))));
> + (input_location, RECUR (TREE_OPERAND (t, 0)), complain));
>
> case CONSTRUCTOR:
> {
> diff --git a/gcc/testsuite/g++.dg/cpp0x/sfinae69.C b/gcc/testsuite/g++.dg/cpp0x/sfinae69.C
> new file mode 100644
> index 00000000000..c4896050b9d
> --- /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<class T>
> +constexpr auto is_throwable(T t) -> decltype(throw t, true) {
> + return true;
> +}
> +template<class T>
> +constexpr bool is_throwable(...) { return false; }
> +
> +constexpr bool b = is_throwable<moveonly>(moveonly{});
> diff --git a/libcc1/libcp1plugin.cc b/libcc1/libcp1plugin.cc
> index 7f4e4c98060..0eff7c68d29 100644
> --- a/libcc1/libcp1plugin.cc
> +++ b/libcc1/libcp1plugin.cc
> @@ -2640,7 +2640,7 @@ plugin_build_unary_expr (cc1_plugin::connection *self,
> break;
>
> case THROW_EXPR:
> - result = build_throw (input_location, op0);
> + result = build_throw (input_location, op0, tf_error);
> break;
>
> case TYPEID_EXPR:
> @@ -2664,7 +2664,7 @@ plugin_build_unary_expr (cc1_plugin::connection *self,
> result = make_pack_expansion (op0);
> break;
>
> - // We're using this for sizeof...(pack). */
> + /* We're using this for sizeof...(pack). */
> case TYPE_PACK_EXPANSION:
> result = make_pack_expansion (op0);
> PACK_EXPANSION_SIZEOF_P (result) = true;
>
> base-commit: 5fb204aaf34b68c427f5b2bfb933fed72fe3eafb
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<class T>
> > +constexpr auto is_throwable(T t) -> decltype(throw t, true) {
> > + return true;
> > +}
> > +template<class T>
> > +constexpr bool is_throwable(...) { return false; }
> > +
> > +constexpr bool b = is_throwable<moveonly>(moveonly{});
Marek
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
@@ -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);
@@ -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);
@@ -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,
+ tf_warning_or_error);
/* Disable all warnings for the generated rethrow statement. */
suppress_warning (rethrow);
finish_expr_stmt (rethrow);
@@ -607,7 +608,7 @@ wrap_cleanups_r (tree *tp, int *walk_subtrees, void * /*data*/)
/* Build a throw expression. */
tree
-build_throw (location_t loc, tree exp)
+build_throw (location_t loc, tree exp, tsubst_flags_t complain)
{
if (exp == error_mark_node)
return exp;
@@ -621,7 +622,7 @@ build_throw (location_t loc, tree exp)
return exp;
}
- if (exp && null_node_p (exp))
+ if (exp && null_node_p (exp) && (complain & tf_warning))
warning_at (loc, 0,
"throwing NULL, which has integral, not pointer type");
@@ -642,8 +643,6 @@ build_throw (location_t loc, tree exp)
tree object, ptr;
tree allocate_expr;
- tsubst_flags_t complain = tf_warning_or_error;
-
/* The CLEANUP_TYPE is the internal type of a destructor. */
if (!cleanup_type)
{
@@ -714,7 +713,6 @@ build_throw (location_t loc, tree exp)
if (CLASS_TYPE_P (temp_type))
{
int flags = LOOKUP_NORMAL | LOOKUP_ONLYCONVERTING;
- bool converted = false;
location_t exp_loc = cp_expr_loc_or_loc (exp, loc);
/* Under C++0x [12.8/16 class.copy], a thrown lvalue is sometimes
@@ -727,23 +725,20 @@ build_throw (location_t loc, tree exp)
exp = moved;
/* Call the copy constructor. */
- if (!converted)
- {
- releasing_vec exp_vec (make_tree_vector_single (exp));
- exp = (build_special_member_call
- (object, complete_ctor_identifier, &exp_vec,
- TREE_TYPE (object), flags, tf_warning_or_error));
- }
-
+ releasing_vec exp_vec (make_tree_vector_single (exp));
+ exp = build_special_member_call (object, complete_ctor_identifier,
+ &exp_vec, TREE_TYPE (object), flags,
+ complain);
if (exp == error_mark_node)
{
- inform (exp_loc, " in thrown expression");
+ if (complain & tf_error)
+ inform (exp_loc, " in thrown expression");
return error_mark_node;
}
}
else
{
- tree tmp = decay_conversion (exp, tf_warning_or_error);
+ tree tmp = decay_conversion (exp, complain);
if (tmp == error_mark_node)
return error_mark_node;
exp = cp_build_init_expr (object, tmp);
@@ -768,7 +763,7 @@ build_throw (location_t loc, tree exp)
tree binfo = TYPE_BINFO (TREE_TYPE (object));
tree dtor_fn = lookup_fnfields (binfo,
complete_dtor_identifier, 0,
- tf_warning_or_error);
+ complain);
dtor_fn = BASELINK_FUNCTIONS (dtor_fn);
if (!mark_used (dtor_fn)
|| !perform_or_defer_access_check (binfo, dtor_fn,
@@ -785,7 +780,7 @@ build_throw (location_t loc, tree exp)
cleanup = build_int_cst (cleanup_type, 0);
/* ??? Indicate that this function call throws throw_type. */
- tree tmp = cp_build_function_call_nary (throw_fn, tf_warning_or_error,
+ tree tmp = cp_build_function_call_nary (throw_fn, complain,
ptr, throw_type, cleanup,
NULL_TREE);
@@ -807,7 +802,7 @@ build_throw (location_t loc, tree exp)
/* ??? Indicate that this function call allows exceptions of the type
of the enclosing catch block (if known). */
- exp = cp_build_function_call_vec (rethrow_fn, NULL, tf_warning_or_error);
+ exp = cp_build_function_call_vec (rethrow_fn, NULL, complain);
}
exp = build1_loc (loc, THROW_EXPR, void_type_node, exp);
@@ -29310,7 +29310,7 @@ cp_parser_throw_expression (cp_parser* parser)
the end at the end of the final token we consumed. */
location_t combined_loc = make_location (start_loc, start_loc,
parser->lexer);
- expression = build_throw (combined_loc, expression);
+ expression = build_throw (combined_loc, expression, tf_warning_or_error);
return expression;
}
@@ -21180,7 +21180,7 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl)
case THROW_EXPR:
RETURN (build_throw
- (input_location, RECUR (TREE_OPERAND (t, 0))));
+ (input_location, RECUR (TREE_OPERAND (t, 0)), complain));
case CONSTRUCTOR:
{
new file mode 100644
@@ -0,0 +1,16 @@
+// PR c++/98388
+// { dg-do compile { target c++11 } }
+
+struct moveonly {
+ moveonly() = default;
+ moveonly(moveonly&&) = default;
+};
+
+template<class T>
+constexpr auto is_throwable(T t) -> decltype(throw t, true) {
+ return true;
+}
+template<class T>
+constexpr bool is_throwable(...) { return false; }
+
+constexpr bool b = is_throwable<moveonly>(moveonly{});
@@ -2640,7 +2640,7 @@ plugin_build_unary_expr (cc1_plugin::connection *self,
break;
case THROW_EXPR:
- result = build_throw (input_location, op0);
+ result = build_throw (input_location, op0, tf_error);
break;
case TYPEID_EXPR:
@@ -2664,7 +2664,7 @@ plugin_build_unary_expr (cc1_plugin::connection *self,
result = make_pack_expansion (op0);
break;
- // We're using this for sizeof...(pack). */
+ /* We're using this for sizeof...(pack). */
case TYPE_PACK_EXPANSION:
result = make_pack_expansion (op0);
PACK_EXPANSION_SIZEOF_P (result) = true;