[PATCH] D153701: [WIP][Clang] Implement P2718R0 "Lifetime extension in range-based for loops"

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:8901-8914
+  // [P2718R0] Lifetime extension in range-based for loops.
+  //
+  // 6.7.7 [class.temporary] p5:
+  // There are four contexts in which temporaries are destroyed at a different
+  // point than the end of the full-expression.
+  //
+  // 6.7.7 [class.temporary] p6:
----------------
yronglin wrote:
> yronglin wrote:
> > hubert.reinterpretcast wrote:
> > > yronglin wrote:
> > > > rsmith wrote:
> > > > > This isn't the right way to model the behavior here -- the presence or absence of an `ExprWithCleanups` is just a convenience to tell consumers of the AST whether they should expect to see cleanups later or not, and doesn't carry an implication of affecting the actual temporary lifetimes and storage durations.
> > > > > 
> > > > > The outcome that we should be aiming to reach is that all `MaterializeTemporaryExpr`s created as part of processing the for-range-initializer are marked as being lifetime-extended by the for-range variable. Probably the simplest way to handle that would be to track the current enclosing for-range-initializer variable in the `ExpressionEvaluationContextRecord`, and whenever a `MaterializeTemporaryExpr` is created, if there is a current enclosing for-range-initializer, mark that `MaterializeTemporaryExpr` as being lifetime-extended by it.
> > > > Awesome! Thanks a lot for your advice, this is very helpful! I want to take a longer look at it.
> > > As mentioned in D139586, `CXXDefaultArgExpr`s may need additional handling. Similarly for `CXXDefaultInitExpr`s.
> > Thanks for your tips! I have a question that what's the correct way to extent the lifetime of `CXXBindTemporaryExpr`? Can I just `materialize` the temporary? It may replaced by `MaterializeTemporaryExpr`, and then I can mark it as being lifetime-extended by the for-range variable.
> Eg.
> ```
> void f() {
>   int v[] = {42, 17, 13};
>   Mutex m;
>   for (int x : static_cast<void>(LockGuard(m)), v) // lock released in C++ 2020
>   {
>     LockGuard guard(m); // OK in C++ 2020, now deadlocks
>   }
> }
> ```
> ```
> BinaryOperator 0x135036220 'int[3]' lvalue ','
> |-CXXStaticCastExpr 0x1350361d0 'void' static_cast<void> <ToVoid>
> | `-CXXFunctionalCastExpr 0x135036198 'LockGuard':'struct LockGuard' functional cast to LockGuard <ConstructorConversion>
> |   `-CXXBindTemporaryExpr 0x135036178 'LockGuard':'struct LockGuard' (CXXTemporary 0x135036178)
> |     `-CXXConstructExpr 0x135036140 'LockGuard':'struct LockGuard' 'void (Mutex &)'
> |       `-DeclRefExpr 0x135035e18 'Mutex':'class Mutex' lvalue Var 0x135035b40 'm' 'Mutex':'class Mutex'
> `-DeclRefExpr 0x135036200 'int[3]' lvalue Var 0x135035928 'v' 'int[3]'
> ```
If `MaterializeTemporaryExpr` represents a "temporary materialization conversion", then the above should already have one just under the `static_cast` to `void` (since the cast operand would be a discarded-value expression).

There may be unfortunate effects from materializing temporaries for discarded-value expressions though: Technically, temporaries are also created for objects having scalar type.

Currently, `MaterializeTemporaryExpr` is documented as being tied to reference binding, but that is not correct: for example, `MaterializeTemporaryExpr` also appears when a member access is made on a temporary of class type.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153701/new/

https://reviews.llvm.org/D153701