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?
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?
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?
And if one can override -std=c++23 -fno-range-based-for-ext-temps,
shall it use the C++17 version?
> > @@ -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.
Jakub
