Re: [PATCH] c++: Implement C++23 P2718R0 - Wording for P2644R1 Fix for Range-based for Loop [PR107637]

[Jason Merrill]

On 9/23/24 9:24 PM, Jakub Jelinek wrote:
On Mon, Sep 23, 2024 at 11:32:59AM -0400, Jason Merrill wrote:
On 8/9/24 9:06 PM, Jakub Jelinek wrote:
Hi!
The following patch implements the C++23 P2718R0 paper
- Wording for P2644R1 Fix for Range-based for Loop.
As all the temporaries from __for_range initialization should have life
extended until the end of __for_range scope, this patch disables (for C++23
and later only and if !processing_template_decl) CLEANUP_POINT_EXPR wrapping
of the __for_range declaration, also disables -Wdangling-reference warning
as well as the rest of extend_ref_init_temps (we know the __for_range temporary
is not TREE_STATIC and as all the temporaries from the initializer will be life
extended, we shouldn't try to handle temporaries referenced by references any
differently) and adds an extra push_stmt_list/pop_stmt_list before
cp_finish_decl of __for_range and after end of the for body and wraps all
that into CLEANUP_POINT_EXPR.
I had to repeat that also for OpenMP range loops because those are handled
differently.

Let's add a flag for this, not just control it with cxx_dialect.  We might
want to consider enabling it by default in earlier modes when not being
strictly conforming?

-frange-based-for-ext-temps
or do you have better suggestion?

I'd probably drop "based", "range-for" seems enough.

Shall we allow also disabling it in C++23 or later modes, or override
user choice unconditionally for C++23+ and only allow users to
enable/disable it in C++11-C++20?

Hmm, I think the latter.

What about the __cpp_range_based_for predefined macro?
Shall it be defined to the C++23 202211L value if the switch is on?
While that could be done in theory for C++17 and later code, for C++11/14
__cpp_range_based_for is 200907L and doesn't include the C++17
201603L step.  Or keep the macro only for C++23 and later?

I think update the macro for 17 and later.

@@ -44600,11 +44609,14 @@ cp_convert_omp_range_for (tree &this_pre
         else
        {
          range_temp = build_range_temp (init);
+         tree name = DECL_NAME (range_temp);
          DECL_NAME (range_temp) = NULL_TREE;
          pushdecl (range_temp);
+         DECL_NAME (range_temp) = name;
          cp_finish_decl (range_temp, init,
                          /*is_constant_init*/false, NULL_TREE,
                          LOOKUP_ONLYCONVERTING);
+         DECL_NAME (range_temp) = NULL_TREE;

This messing with the name needs a rationale.  What wants it to be null?

I'll add comments.  The first = NULL_TREE; is needed so that pushdecl
doesn't register the temporary for name lookup, the = name now is so that
cp_finish_decl recognizes the temporary as range based for temporary
for the lifetime extension, and the last one is just to preserve previous
behavior, not have it visible in debug info etc.

But cp_convert_range_for doesn't ever set the name to NULL_TREE, why 
should the OMP variant be different?

Having it visible to name lookup in the debugger seems beneficial. 
Having it visible to the code seems less useful, but not important to 
prevent.

Jason