Jason Merrill <ja...@redhat.com> writes: > On 8/7/24 7:31 PM, Arsen Arsenović wrote: >> Enlargening the function-specific data block is not great. > > Indeed, I think it would be better to search DECL_SAVED_TREE for a RETURN_STMT > once we've decided to give an error.
The trouble with that is that finish_return_stmt currently uses
input_location as the location for the entire return expr, so the
location ends up being after the entire return value.
I've hacked in a way to provide a different location to
finih_return_stmt, when applying it like below, the produced result is
the first result in the original email:
diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index 6cfe42f3bdd6..44b45f16b026 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -14965,12 +14965,15 @@ cp_parser_jump_statement (cp_parser* parser, tree
&std_attrs)
/* Build the return-statement, check co-return first, since type
deduction is not valid there. */
+ auto l = make_location (token->location,
+ token->location,
+ input_location);
if (keyword == RID_CO_RETURN)
statement = finish_co_return_stmt (token->location, expr);
else if (FNDECL_USED_AUTO (current_function_decl) && in_discarded_stmt)
/* Don't deduce from a discarded return statement. */;
else
- statement = finish_return_stmt (expr);
+ statement = finish_return_stmt (expr, l);
/* Look for the final `;'. */
cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON);
}
... without this change (so, using input_location), the result is:
test.cc:38:11: error: a ‘return’ statement is not allowed in coroutine; did you
mean ‘co_return’?
38 | return {};
| ^
... which is not the best. That's the change I'm referring to in the
original post that I haven't ran the testsuite on. Changing that
location allows for simply searching DECL_SAVED_TREE (fndecl), though,
and getting a good location out of it.
>> I've
>> considered changing the location of RETURN_STMT expressions to cover
>> everything from the return expression to input_location after parsing
>> the returned expr. The result of that is:
>> test.cc:38:3: error: a ‘return’ statement is not allowed in coroutine; did
>> you mean ‘co_return’?
>> 38 | return {};
>> | ^~~~~~~~~
>> test.cc:37:3: note: function was made a coroutine here
>> 37 | co_return;
>> | ^~~~~~~~~
>> ... so, not bad, but I'm not sure how intrusive such a change would be
>> (haven't tried the testsuite). The current patch produces:
>> test.cc:36:3: error: a ‘return’ statement is not allowed in coroutine; did
>> you mean ‘co_return’?
>> 36 | return {};
>> | ^~~~~~
>> test.cc:35:3: note: function was made a coroutine here
>> 35 | co_return;
>> | ^~~~~~~~~
>> Is there a better location to use here or is the current (latter) one
>> OK?
>
> The latter seems fine.
>
>> I haven't managed to found a nicer existing one. We also can't
>> stash it in coroutine_info because a function might not have that at
>> time we parse a return.
>> Tested on x86_64-pc-linux-gnu.
>> Have a lovely evening.
>> ---------- >8 ----------
>> We now point out why a function is a coroutine.
>> gcc/cp/ChangeLog:
>> * coroutines.cc (coro_function_valid_p): Change how we diagnose
>> returning coroutines.
>> * cp-tree.h (struct language_function): Add first_return_loc
>> field. Tracks the location of the first return encountered
>> during parsing.
>> (current_function_first_return_loc): New macro. Expands to the
>> current functions' first_return_loc.
>> * parser.cc (cp_parser_jump_statement): If parsing a RID_RETURN,
>> save its location to current_function_first_return_loc.
>> gcc/testsuite/ChangeLog:
>> * g++.dg/coroutines/co-return-syntax-08-bad-return.C: Update to
>> match new diagnostic.
>> ---
>> gcc/cp/coroutines.cc | 14 +++--
>> gcc/cp/cp-tree.h | 6 +++
>> gcc/cp/parser.cc | 4 ++
>> .../co-return-syntax-08-bad-return.C | 52 +++++++++++++++++--
>> 4 files changed, 68 insertions(+), 8 deletions(-)
>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>> index 0f4dc42ec1c8..f32c7a2eec8d 100644
>> --- a/gcc/cp/coroutines.cc
>> +++ b/gcc/cp/coroutines.cc
>> @@ -968,11 +968,15 @@ coro_function_valid_p (tree fndecl)
>> if (current_function_returns_value || current_function_returns_null)
>> {
>> - /* TODO: record or extract positions of returns (and the first coro
>> - keyword) so that we can add notes to the diagnostic about where
>> - the bad keyword is and what made the function into a coro. */
>> - error_at (f_loc, "a %<return%> statement is not allowed in coroutine;"
>> - " did you mean %<co_return%>?");
>> + coroutine_info *coro_info = get_or_insert_coroutine_info (fndecl);
>> + auto retloc = current_function_first_return_loc;
>> + gcc_checking_assert (retloc && coro_info->first_coro_keyword);
>> +
>> + auto_diagnostic_group diaggrp;
>> + error_at (retloc, "a %<return%> statement is not allowed in
>> coroutine;"
>> + " did you mean %<co_return%>?");
>> + inform (coro_info->first_coro_keyword,
>> + "function was made a coroutine here");
>> return false;
>> }
>> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
>> index 911d1d7924cc..68c681150a1f 100644
>> --- a/gcc/cp/cp-tree.h
>> +++ b/gcc/cp/cp-tree.h
>> @@ -2123,6 +2123,8 @@ struct GTY(()) language_function {
>> tree x_vtt_parm;
>> tree x_return_value;
>> + location_t first_return_loc;
>> +
>> BOOL_BITFIELD returns_value : 1;
>> BOOL_BITFIELD returns_null : 1;
>> BOOL_BITFIELD returns_abnormally : 1;
>> @@ -2217,6 +2219,10 @@ struct GTY(()) language_function {
>> #define current_function_return_value \
>> (cp_function_chain->x_return_value)
>> +/* Location of the first 'return' stumbled upon during parsing. */
>> +
>> +#define current_function_first_return_loc
>> cp_function_chain->first_return_loc
>> +
>> /* In parser.cc. */
>> extern tree cp_literal_operator_id (const char *);
>> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
>> index eb102dea8299..6cfe42f3bdd6 100644
>> --- a/gcc/cp/parser.cc
>> +++ b/gcc/cp/parser.cc
>> @@ -14957,6 +14957,10 @@ cp_parser_jump_statement (cp_parser* parser, tree
>> &std_attrs)
>> AGGR_INIT_EXPR_MUST_TAIL (ret_expr) = musttail_p;
>> else
>> set_musttail_on_return (expr, token->location, musttail_p);
>> +
>> + /* Save where we saw this keyword. */
>> + if (current_function_first_return_loc == UNKNOWN_LOCATION)
>> + current_function_first_return_loc = token->location;
>> }
>> /* Build the return-statement, check co-return first, since type
>> diff --git
>> a/gcc/testsuite/g++.dg/coroutines/co-return-syntax-08-bad-return.C
>> b/gcc/testsuite/g++.dg/coroutines/co-return-syntax-08-bad-return.C
>> index 148ee4543e87..1e5d9e7a65a1 100644
>> --- a/gcc/testsuite/g++.dg/coroutines/co-return-syntax-08-bad-return.C
>> +++ b/gcc/testsuite/g++.dg/coroutines/co-return-syntax-08-bad-return.C
>> @@ -27,6 +27,7 @@ struct Coro {
>> auto final_suspend () noexcept { return coro::suspend_always{}; }
>> void return_void () { }
>> void unhandled_exception() { }
>> + auto yield_value (auto) noexcept { return coro::suspend_always{}; }
>> };
>> };
>> @@ -34,10 +35,55 @@ extern int x;
>> // Diagnose disallowed "return" in coroutine.
>> Coro
>> -bar () // { dg-error {a 'return' statement is not allowed} }
>> +bar ()
>> {
>> if (x)
>> - return Coro();
>> + return Coro(); // { dg-error {a 'return' statement is not allowed} }
>> else
>> - co_return;
>> + co_return; // { dg-note "function was made a coroutine here" }
>> +}
>> +
>> +Coro
>> +bar1 ()
>> +{
>> + if (x)
>> + return Coro(); // { dg-error {a 'return' statement is not allowed} }
>> + else
>> + co_await std::suspend_never{}; // { dg-note "function was made a
>> coroutine here" }
>> +}
>> +
>> +Coro
>> +bar2 ()
>> +{
>> + if (x)
>> + return Coro(); // { dg-error {a 'return' statement is not allowed} }
>> + else
>> + co_yield 123; // { dg-note "function was made a coroutine here" }
>> +}
>> +
>> +Coro
>> +bar3 ()
>> +{
>> + if (x)
>> + co_return; // { dg-note "function was made a coroutine here" }
>> + else
>> + return Coro(); // { dg-error {a 'return' statement is not allowed} }
>> +}
>> +
>> +Coro
>> +bar4 ()
>> +{
>> + if (x)
>> + co_await std::suspend_never{}; // { dg-note "function was made a
>> coroutine here" }
>> + else
>> + return Coro(); // { dg-error {a 'return' statement is not allowed} }
>> +}
>> +
>> +Coro
>> +bar5 ()
>> +{
>> + if (x)
>> + co_yield 123; // { dg-note "function was made a coroutine here" }
>> + else
>> + return Coro(); // { dg-error {a 'return' statement is not allowed} }
>> }
--
Arsen Arsenović
signature.asc
Description: PGP signature
