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

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 549715.
yronglin added a comment.

Handle default argument and dependent context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/AST/ast-dump-for-range-lifetime.cpp

Index: clang/test/AST/ast-dump-for-range-lifetime.cpp
===
--- /dev/null
+++ clang/test/AST/ast-dump-for-range-lifetime.cpp
@@ -0,0 +1,355 @@
+// RUN: %clang_cc1 -std=c++23 -triple x86_64-linux-gnu -fcxx-exceptions -ast-dump %s \
+// RUN: | FileCheck -strict-whitespace %s
+
+namespace P2718R0 {
+
+// Test basic
+struct A {
+  int a[3] = {1, 2, 3};
+  A() {}
+  ~A() {}
+  const int *begin() const { return a; }
+  const int *end() const { return a + 3; }
+  A& r() { return *this; }
+  A g() { return A(); }
+};
+
+A g() { return A(); }
+const A (const A ) { return t; }
+
+void test1() {
+  [[maybe_unused]] int sum = 0;
+  // CHECK: FunctionDecl {{.*}} test1 'void ()'
+  // CHECK:  |   `-CXXForRangeStmt {{.*}}
+  // CHECK-NEXT: | |-<<>>
+  // CHECK-NEXT: | |-DeclStmt {{.*}}
+  // CHECK-NEXT: | | `-VarDecl {{.*}} implicit used __range1 'const A &' cinit
+  // CHECK-NEXT: | |   `-ExprWithCleanups {{.*}} 'const A':'const P2718R0::A' lvalue
+  // CHECK-NEXT: | | `-CallExpr {{.*}} 'const A':'const P2718R0::A' lvalue
+  // CHECK-NEXT: | |   |-ImplicitCastExpr {{.*}} 'const A &(*)(const A &)' 
+  // CHECK-NEXT: | |   | `-DeclRefExpr {{.*}} 'const A &(const A &)' lvalue Function {{.*}} 'f1' 'const A &(const A &)'
+  // CHECK-NEXT: | |   `-MaterializeTemporaryExpr {{.*}} 'const A':'const P2718R0::A' lvalue extended by Var {{.*}} '__range1' 'const A &'
+  // CHECK-NEXT: | | `-ImplicitCastExpr {{.*}} 'const A':'const P2718R0::A' 
+  // CHECK-NEXT: | |   `-CXXBindTemporaryExpr {{.*}} 'A':'P2718R0::A' (CXXTemporary {{.*}})
+  // CHECK-NEXT: | | `-CallExpr {{.*}} 'A':'P2718R0::A'
+  // CHECK-NEXT: | |   `-ImplicitCastExpr {{.*}} 'A (*)()' 
+  // CHECK-NEXT: | | `-DeclRefExpr {{.*}} 'A ()' lvalue Function {{.*}} 'g' 'A ()'
+  for (auto e : f1(g()))
+sum += e;
+}
+
+struct B : A {};
+int ((const A *))[3];
+const A *g(const A &);
+void bar(int) {}
+
+void test2() {
+  // CHECK: FunctionDecl {{.*}} test2 'void ()'
+  // CHECK:  |   `-CXXForRangeStmt {{.*}}
+  // CHECK-NEXT: | |-<<>>
+  // CHECK-NEXT: | |-DeclStmt {{.*}}
+  // CHECK-NEXT: | | `-VarDecl {{.*}} implicit used __range1 'int (&)[3]' cinit
+  // CHECK-NEXT: | |   `-ExprWithCleanups {{.*}} 'int[3]':'int[3]' lvalue
+  // CHECK-NEXT: | | `-CallExpr {{.*}} 'int[3]':'int[3]' lvalue
+  // CHECK-NEXT: | |   |-ImplicitCastExpr {{.*}} 'int (&(*)(const A *))[3]' 
+  // CHECK-NEXT: | |   | `-DeclRefExpr {{.*}} 'int (&(const A *))[3]' lvalue Function {{.*}} 'f' 'int (&(const A *))[3]'
+  // CHECK-NEXT: | |   `-CallExpr {{.*}} 'const A *'
+  // CHECK-NEXT: | | |-ImplicitCastExpr {{.*}} 'const A *(*)(const A &)' 
+  // CHECK-NEXT: | | | `-DeclRefExpr {{.*}} 'const A *(const A &)' lvalue Function {{.*}} 'g' 'const A *(const A &)'
+  // CHECK-NEXT: | | `-ImplicitCastExpr {{.*}} 'const A':'const P2718R0::A' lvalue 
+  // CHECK-NEXT: | |   `-MaterializeTemporaryExpr {{.*}} 'const B':'const P2718R0::B' lvalue extended by Var {{.*}} '__range1' 'int (&)[3]'
+  // CHECK-NEXT: | | `-ImplicitCastExpr {{.*}} 'const B':'const P2718R0::B' 
+  // CHECK-NEXT: | |   `-CXXBindTemporaryExpr {{.*}} 'B':'P2718R0::B' (CXXTemporary {{.*}})
+  // CHECK-NEXT: | | `-CXXTemporaryObjectExpr {{.*}} 'B':'P2718R0::B' 'void () noexcept(false)' zeroing
+  for (auto e : f(g(B(
+bar(e);
+}
+
+// Test discard statement.
+struct LockGuard {
+LockGuard() {}
+~LockGuard() {}
+};
+
+void test3() {
+  int v[] = {42, 17, 13};
+
+  // CHECK: FunctionDecl {{.*}} test3 'void ()'
+  // CHECK:  -CXXForRangeStmt {{.*}}
+  // CHECK-NEXT:  |-<<>>
+  // CHECK-NEXT:  |-DeclStmt {{.*}}
+  // CHECK-NEXT:  | `-VarDecl {{.*}} implicit used __range1 'int (&)[3]' cinit
+  // CHECK-NEXT:  |   `-ExprWithCleanups {{.*}} 'int[3]' lvalue
+  // CHECK-NEXT:  | `-BinaryOperator {{.*}} 'int[3]' lvalue ','
+  // CHECK-NEXT:  |   |-CXXStaticCastExpr {{.*}} 'void' static_cast 
+  // CHECK-NEXT:  |   | `-MaterializeTemporaryExpr {{.*}} 'LockGuard':'P2718R0::LockGuard' xvalue extended by Var {{.*}} '__range1' 'int (&)[3]'
+  // CHECK-NEXT:  |   |   `-CXXBindTemporaryExpr {{.*}} 'LockGuard':'P2718R0::LockGuard' (CXXTemporary {{.*}})
+  // 

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

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 549449.
yronglin added a comment.

Only create addational metarialized temporary in for-range-init.
FIXME: Need to handle function default argument, and add more test to make sure 
the generated LLVM IR is correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/AST/ast-dump-for-range-lifetime.cpp

Index: clang/test/AST/ast-dump-for-range-lifetime.cpp
===
--- /dev/null
+++ clang/test/AST/ast-dump-for-range-lifetime.cpp
@@ -0,0 +1,68 @@
+// RUN: %clang_cc1 -std=c++23 -triple x86_64-linux-gnu -fcxx-exceptions -ast-dump %s \
+// RUN: | FileCheck -strict-whitespace %s
+
+namespace p2718r0 {
+struct T {
+  int a[10] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10};
+  T() {}
+  ~T() {}
+  const int *begin() const { return a; }
+  const int *end() const { return a + 10; }
+};
+
+const T (const T ) { return t; }
+T g() { return T(); }
+
+void foo() {
+  // CHECK: FunctionDecl {{.*}} foo 'void ()'
+  // CHECK: `-CXXForRangeStmt {{.*}}
+  // CHECK-NEXT:|-<<>>
+  // CHECK-NEXT:|-DeclStmt {{.*}}
+  // CHECK-NEXT:| `-VarDecl {{.*}} implicit used __range1 'const T &' cinit
+  // CHECK-NEXT:|   `-ExprWithCleanups {{.*}} 'const T':'const p2718r0::T' lvalue
+  // CHECK-NEXT:| `-CallExpr {{.*}} 'const T':'const p2718r0::T' lvalue
+  // CHECK-NEXT:|   |-ImplicitCastExpr {{.*}} 'const T &(*)(const T &)' 
+  // CHECK-NEXT:|   | `-DeclRefExpr {{.*}} 'const T &(const T &)' lvalue Function {{.*}} 'f1' 'const T &(const T &)'
+  // CHECK-NEXT:|   `-MaterializeTemporaryExpr {{.*}} 'const T':'const p2718r0::T' lvalue extended by Var {{.*}} '__range1' 'const T &'
+  // CHECK-NEXT:| `-ImplicitCastExpr {{.*}} 'const T':'const p2718r0::T' 
+  // CHECK-NEXT:|   `-CXXBindTemporaryExpr {{.*}}  'T':'p2718r0::T' (CXXTemporary {{.*}})
+  // CHECK-NEXT:| `-CallExpr {{.*}} 'T':'p2718r0::T'
+  // CHECK-NEXT:|   `-ImplicitCastExpr {{.*}} 'T (*)()' 
+  // CHECK-NEXT:| `-DeclRefExpr {{.*}} 'T ()' lvalue Function {{.*}} 'g' 'T ()'
+  [[maybe_unused]] int sum = 0;
+  for (auto e : f1(g()))
+sum += e;
+}
+
+struct LockGuard {
+LockGuard(int) {}
+
+~LockGuard() {}
+};
+
+void f() {
+  int v[] = {42, 17, 13};
+  int M = 0;
+
+  // CHECK: FunctionDecl {{.*}} f 'void ()'
+  // CHECK: `-CXXForRangeStmt {{.*}}
+  // CHECK-NEXT:   |-<<>>
+  // CHECK-NEXT:   |-DeclStmt {{.*}}
+  // CHECK-NEXT:   | `-VarDecl {{.*}} col:16 implicit used __range1 'int (&)[3]' cinit
+  // CHECK-NEXT:   |   `-ExprWithCleanups {{.*}} 'int[3]' lvalue
+  // CHECK-NEXT:   | `-BinaryOperator {{.*}} 'int[3]' lvalue ','
+  // CHECK-NEXT:   |   |-CXXStaticCastExpr {{.*}}'void' static_cast 
+  // CHECK-NEXT:   |   | `-MaterializeTemporaryExpr {{.*}} 'LockGuard':'p2718r0::LockGuard' xvalue extended by Var {{.*}} '__range1' 'int (&)[3]'
+  // CHECK-NEXT:   |   |   `-CXXFunctionalCastExpr {{.*}} 'LockGuard':'p2718r0::LockGuard' functional cast to LockGuard 
+  // CHECK-NEXT:   |   | `-CXXBindTemporaryExpr {{.*}} 'LockGuard':'p2718r0::LockGuard' (CXXTemporary {{.*}})
+  // CHECK-NEXT:   |   |   `-CXXConstructExpr {{.*}} 'LockGuard':'p2718r0::LockGuard' 'void (int)'
+  // CHECK-NEXT:   |   | `-ImplicitCastExpr {{.*}} 'int' 
+  // CHECK-NEXT:   |   |   `-DeclRefExpr {{.*}} 'int' lvalue Var {{.*}} 'M' 'int'
+  // CHECK-NEXT:   |   `-DeclRefExpr {{.*}} 'int[3]' lvalue Var {{.*}} 'v' 'int[3]'
+  for (int x : static_cast(LockGuard(M)), v) // lock released in C++ 2020
+  {
+LockGuard guard(M); // OK in C++ 2020, now deadlocks
+  }
+}
+
+} // namespace p2718r0
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -27,6 +27,7 @@
 #include "clang/AST/TypeOrdering.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Lex/Preprocessor.h"
+#include "clang/Sema/EnterExpressionEvaluationContext.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Ownership.h"
@@ -2529,10 +2530,20 @@
   VarDecl *RangeVar = BuildForRangeVarDecl(*this, RangeLoc,
Context.getAutoRRefDeductType(),
std::string("__range") + DepthStr);
-  if (FinishForRangeVarDecl(*this, RangeVar, Range, RangeLoc,
-diag::err_for_range_deduction_failure)) {
-ActOnInitializerError(LoopVar);
-return StmtError();
+  {
+EnterExpressionEvaluationContext RangeVarContext(
+*this, 

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

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

In D153701#4575056 , 
@hubert.reinterpretcast wrote:

> In D153701#4563919 , @yronglin 
> wrote:
>
>> The gap between these two numbers is very large. So I'think we can create 
>> additional materializations only within for-range initializers. I'm not sure 
>> if I can find a way to only create materializes for temporaries that need to 
>> have an extended lifetime, WDYT?
>
> @rsmith asked for something slightly different (but I am not sure how to 
> collect that info): He wanted the AST size delta.
>
> For what we have, I think the detailed results table would be more useful if 
> we add some size metric for the files and the increase expressed as a 
> percentage. Perhaps sorting by file size would help contextualize the 
> significance of the percentage change.

Thanks to Aaron's suggestion in Discard, the initial idea was to count the 
number of `MaterializedTemporaryExpr` created in the process of building 
llvm-project, and then calculate the precise memory increment data, without 
being affected by the operating system state. Maybe I can refine this table, 
add a column to show the memory increment  percentage change of each file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D153701#4563919 , @yronglin wrote:

> The gap between these two numbers is very large. So I'think we can create 
> additional materializations only within for-range initializers. I'm not sure 
> if I can find a way to only create materializes for temporaries that need to 
> have an extended lifetime, WDYT?

@rsmith asked for something slightly different (but I am not sure how to 
collect that info): He wanted the AST size delta.

For what we have, I think the detailed results table would be more useful if we 
add some size metric for the files and the increase expressed as a percentage. 
Perhaps sorting by file size would help contextualize the significance of the 
percentage change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

Sorry for the late reply.  I tried to investigate the memory impact of creating 
these additional materializations by build the whole llvm-project and compare 
the number of `MaterializedTemporaryExpr` created during parsing.

Steps:

1. Add a public member `uint64_t NumMetarilizedTemporaryExpr = 0;` in 
`ASTContext` .
2. Increment the value of `NumMetarilizedTemporaryExpr ` in 
`Sema::CreateMaterializeTemporaryExpr`.
3. Write the `NumMetarilizedTemporaryExpr ` into a text file when `ASTContext` 
destruction, each translation unit will append a line to the file to record the 
value of `NumMetarilizedTemporaryExpr `.
4. Build the entire llvm-project separately using the compiler that creates 
addational materializations and the compiler that doesn't.
5. Sum the numbers produced by each translation unit.

The result is:

The version that create addational materializations: 50740658
The version that does not create addational materializations:  18360888

The gap between these two numbers is very large. So I'think we can create 
additional materializations only within for-range initializers. I'm not sure if 
I can find a way to only create materializes for temporaries that need to have 
an extended lifetime, WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

This is a very early implement, please give me some time to add and fix tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

Thanks for your comments and sorry for the very late reply, I have been 
investigating how to achieve it these days.




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:

rsmith wrote:
> hubert.reinterpretcast wrote:
> > yronglin wrote:
> > > hubert.reinterpretcast wrote:
> > > > yronglin wrote:
> > > > > hubert.reinterpretcast wrote:
> > > > > > 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(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 
> > > > > > > | `-CXXFunctionalCastExpr 0x135036198 'LockGuard':'struct 
> > > > > > > LockGuard' functional cast to LockGuard 
> > > > > > > |   `-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.
> > > > > http://eel.is/c++draft/class.temporary says:
> > > > > ```
> > > > > [Note 3: Temporary objects are materialized:
> > > > > ...
> > > > > (2.6)
> > > > > when a prvalue that has type other than cv void appears as a 
> > > > > discarded-value expression ([expr.context]).
> > > > > — end note]

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

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 544386.
yronglin added a comment.

Try implement


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701

Files:
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp

Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -410,6 +410,7 @@
   case BuildingBuiltinDumpStructCall:
   case LambdaExpressionSubstitution:
   case BuildingDeductionGuides:
+  case BuiltingCXXForRangeVariable:
 return false;
 
   // This function should never be called when Kind's value is Memoization.
@@ -1009,7 +1010,12 @@
   << convertCallArgsToString(
  *this, llvm::ArrayRef(Active->CallArgs, Active->NumCallArgs));
   break;
-
+case CodeSynthesisContext::BuiltingCXXForRangeVariable:
+  Diags.Report(Active->PointOfInstantiation,
+   diag::note_building_builtin_dump_struct_call)
+  << convertCallArgsToString(
+ *this, llvm::ArrayRef(Active->CallArgs, Active->NumCallArgs));
+  break;
 case CodeSynthesisContext::Memoization:
   break;
 
@@ -1138,6 +1144,7 @@
 case CodeSynthesisContext::MarkingClassDllexported:
 case CodeSynthesisContext::BuildingBuiltinDumpStructCall:
 case CodeSynthesisContext::BuildingDeductionGuides:
+case CodeSynthesisContext::BuiltingCXXForRangeVariable:
   // This happens in a context unrelated to template instantiation, so
   // there is no SFINAE.
   return std::nullopt;
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -2522,6 +2522,12 @@
 }
   }
 
+  // Register a note to explain why we're performing the call.
+  CodeSynthesisContext Ctx;
+  Ctx.Kind = CodeSynthesisContext::BuiltingCXXForRangeVariable;
+  Ctx.PointOfInstantiation = ForLoc;
+  pushCodeSynthesisContext(Ctx);
+
   // Build  auto && __range = range-init
   // Divide by 2, since the variables are in the inner scope (loop body).
   const auto DepthStr = std::to_string(S->getDepth() / 2);
@@ -2529,8 +2535,10 @@
   VarDecl *RangeVar = BuildForRangeVarDecl(*this, RangeLoc,
Context.getAutoRRefDeductType(),
std::string("__range") + DepthStr);
-  if (FinishForRangeVarDecl(*this, RangeVar, Range, RangeLoc,
-diag::err_for_range_deduction_failure)) {
+  bool Res = FinishForRangeVarDecl(*this, RangeVar, Range, RangeLoc,
+   diag::err_for_range_deduction_failure);
+  popCodeSynthesisContext();
+  if (Res) {
 ActOnInitializerError(LoopVar);
 return StmtError();
   }
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -7355,15 +7355,15 @@
   });
 }
 
-static void visitLocalsRetainedByInitializer(IndirectLocalPath ,
- Expr *Init, LocalVisitor Visit,
- bool RevisitSubinits,
- bool EnableLifetimeWarnings);
+static void
+visitLocalsRetainedByInitializer(IndirectLocalPath , Expr *Init,
+ LocalVisitor Visit, bool RevisitSubinits,
+ bool EnableLifetimeWarnings,
+ bool isCXXForRangeVariable = false);
 
-static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath ,
-  Expr *Init, ReferenceKind RK,
-  LocalVisitor Visit,
-  bool EnableLifetimeWarnings);
+static void visitLocalsRetainedByReferenceBinding(
+IndirectLocalPath , Expr *Init, ReferenceKind RK, LocalVisitor Visit,
+bool EnableLifetimeWarnings, bool isCXXForRangeVariable = false);
 
 template  static bool isRecordWithAttr(QualType Type) {
   if (auto *RD = Type->getAsCXXRecordDecl())
@@ -7543,7 +7543,8 @@
 }
 
 static void visitLifetimeBoundArguments(IndirectLocalPath , Expr *Call,
-LocalVisitor Visit) {
+LocalVisitor Visit,
+bool isCXXForRangeVariable = false) {
   const FunctionDecl *Callee;
   ArrayRef Args;
 
@@ -7584,7 +7585,8 @@
   for (unsigned I = 0,
 N = 

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith 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:

hubert.reinterpretcast wrote:
> yronglin wrote:
> > hubert.reinterpretcast wrote:
> > > yronglin wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > 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(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 
> > > > > > | `-CXXFunctionalCastExpr 0x135036198 'LockGuard':'struct 
> > > > > > LockGuard' functional cast to LockGuard 
> > > > > > |   `-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.
> > > > http://eel.is/c++draft/class.temporary says:
> > > > ```
> > > > [Note 3: Temporary objects are materialized:
> > > > ...
> > > > (2.6)
> > > > when a prvalue that has type other than cv void appears as a 
> > > > discarded-value expression ([expr.context]).
> > > > — end note]
> > > > ```
> > > > Seems we should materialized the discard-value expression in this case, 
> > > > WDYT?
> > > I think we should, but what is the codegen fallout? Would no-opt builds 
> > > start writing `42` into allocated memory for `static_cast(42)`?
> > Thanks for your confirm @hubert.reinterpretcast ! 
> > 
> 

[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:
> hubert.reinterpretcast wrote:
> > yronglin wrote:
> > > hubert.reinterpretcast wrote:
> > > > 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(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 
> > > > > | `-CXXFunctionalCastExpr 0x135036198 'LockGuard':'struct LockGuard' 
> > > > > functional cast to LockGuard 
> > > > > |   `-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.
> > > http://eel.is/c++draft/class.temporary says:
> > > ```
> > > [Note 3: Temporary objects are materialized:
> > > ...
> > > (2.6)
> > > when a prvalue that has type other than cv void appears as a 
> > > discarded-value expression ([expr.context]).
> > > — end note]
> > > ```
> > > Seems we should materialized the discard-value expression in this case, 
> > > WDYT?
> > I think we should, but what is the codegen fallout? Would no-opt builds 
> > start writing `42` into allocated memory for `static_cast(42)`?
> Thanks for your confirm @hubert.reinterpretcast ! 
> 
> I have tried locally, the generated  IR of `void f()` is:
> ```
> define void @f()() {
> entry:
>   %v = alloca [3 x i32], align 4
>   %m = alloca %class.Mutex, align 8
>   %__range1 = alloca ptr, align 

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

[Yurong via Phabricator via cfe-commits]

yronglin marked 2 inline comments as done.
yronglin 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:

hubert.reinterpretcast wrote:
> yronglin wrote:
> > hubert.reinterpretcast wrote:
> > > 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(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 
> > > > | `-CXXFunctionalCastExpr 0x135036198 'LockGuard':'struct LockGuard' 
> > > > functional cast to LockGuard 
> > > > |   `-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.
> > http://eel.is/c++draft/class.temporary says:
> > ```
> > [Note 3: Temporary objects are materialized:
> > ...
> > (2.6)
> > when a prvalue that has type other than cv void appears as a 
> > discarded-value expression ([expr.context]).
> > — end note]
> > ```
> > Seems we should materialized the discard-value expression in this case, 
> > WDYT?
> I think we should, but what is the codegen fallout? Would no-opt builds start 
> writing `42` into allocated memory for `static_cast(42)`?
Thanks for your confirm @hubert.reinterpretcast ! 

I have tried locally, the generated  IR of `void f()` is:
```
define void @f()() {
entry:
  %v = alloca [3 x i32], align 4
  %m = alloca %class.Mutex, align 8
  %__range1 = alloca ptr, align 8
  %ref.tmp = alloca %struct.LockGuard, align 8
  %__begin1 = alloca ptr, align 8
  %__end1 = alloca ptr, align 8
  %x = alloca i32, align 4
  %guard = alloca 

[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:
> hubert.reinterpretcast wrote:
> > 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(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 
> > > | `-CXXFunctionalCastExpr 0x135036198 'LockGuard':'struct LockGuard' 
> > > functional cast to LockGuard 
> > > |   `-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.
> http://eel.is/c++draft/class.temporary says:
> ```
> [Note 3: Temporary objects are materialized:
> ...
> (2.6)
> when a prvalue that has type other than cv void appears as a discarded-value 
> expression ([expr.context]).
> — end note]
> ```
> Seems we should materialized the discard-value expression in this case, WDYT?
I think we should, but what is the codegen fallout? Would no-opt builds start 
writing `42` into allocated memory for `static_cast(42)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yurong via Phabricator via cfe-commits]

yronglin 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:

hubert.reinterpretcast wrote:
> 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(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 
> > | `-CXXFunctionalCastExpr 0x135036198 'LockGuard':'struct LockGuard' 
> > functional cast to LockGuard 
> > |   `-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.
http://eel.is/c++draft/class.temporary says:
```
[Note 3: Temporary objects are materialized:
...
(2.6)
when a prvalue that has type other than cv void appears as a discarded-value 
expression ([expr.context]).
— end note]
```
Seems we should materialized the discard-value expression in this case, WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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(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 
> | `-CXXFunctionalCastExpr 0x135036198 'LockGuard':'struct LockGuard' 
> functional cast to LockGuard 
> |   `-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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin requested changes to this revision.
cor3ntin added a comment.
This revision now requires changes to proceed.

Marking that as needing revision while the author explores the direction 
outlined by Richard :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yurong via Phabricator via cfe-commits]

yronglin marked an inline comment as done.
yronglin 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:
> 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(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 
| `-CXXFunctionalCastExpr 0x135036198 'LockGuard':'struct LockGuard' functional 
cast to LockGuard 
|   `-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]'
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yurong via Phabricator via cfe-commits]

yronglin marked 2 inline comments as done.
yronglin 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:

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits