> On 24 Jul 2025, at 17:31, Jason Merrill <ja...@redhat.com> wrote:
>
> On 7/24/25 10:24 AM, Iain Sandoe wrote:
>>>>> We should check (gcc_checking_assert?) that NRVO works in the case where
>>>>> we expect it to, rather than let NRVO failures show up as wrong-code.
>>>> This seems a simple request - but it seems quite involved to implement;
>>>> the conditions that have to be met are numerous - I've made an attempt
>>>> (that
>>>> does not regress any of the current testsuite) - but it seems potentially
>>>> fragile.
>>> It should work to check current_function_return_value == gro (after both
>>> returns).
>> Thanks I was not sure that was sufficient; Changed to do this.
>
>> @@ -5423,8 +5432,32 @@ cp_coroutine_transform::build_ramp_function ()
>> /* The ramp is done, we just need the return statement, which we build
>> from
>> the return object we constructed before we called the actor. */
>> + /* This is our 'normal' exit. */
>> r = void_ramp_p ? NULL_TREE : convert_from_reference (coro_gro);
>> finish_return_stmt (r);
>> + /* Check that we did NRV when expected. */
>> + gcc_checking_assert (void_ramp_p
>> + || !same_type_p (gro_type, fn_return_type)
>> + || error_operand_p (current_function_return_value)
>> + || current_function_return_value == coro_gro);
>
> This isn't after both returns, the grooaf return is still below.
But the grooaf return does not use the g_r_o - in general there’s no reason to
expect that the g_r_o_o_a_f returns the same type as g_r_o (which was why I
made a comment about perhaps more wording tweaks are needed)
> Also error_mark_node means no NRV, that's the case we are trying to check
> doesn't happen.
Hmm … that’s tricky - because it can also happen when there’s some other
earlier error with inputs (or the return_decl)?
perhaps we need to bail earlier in that case.
> Maybe we should make want_nrvo_p non-static so you can check !that instead of
> trying to reproduce it here?
I had considered that - if it OK with you then I can investigate this - but I
think we need to be clear on the stuff above too.
> The patch is OK without the assert, we can add it separately.
thanks, ack
Iain
>
>> +
>> + if (grooaf)
>> + {
>> + finish_compound_stmt (alloc_ok_scope);
>> + finish_then_clause (grooaf_if_stmt);
>> +
>> + begin_else_clause (grooaf_if_stmt);
>> + /* We come here if the frame allocation failed. */
>> + r = NULL_TREE;
>> + if (void_ramp_p)
>> + /* Execute the get-return-object-on-alloc-fail call... */
>> + finish_expr_stmt (grooaf);
>> + else
>> + /* Get the fallback return object. */
>> + r = grooaf;
>> + finish_return_stmt (r);
>> + finish_if_stmt (grooaf_if_stmt);
>> + }
>
