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

[Yurong via Phabricator via cfe-commits]

yronglin marked 4 inline comments as done.
yronglin added a comment.

@cor3ntin Thanks for your review!



================
Comment at: clang/include/clang/Sema/Sema.h:1357
+    /// Whether rewrite the default argument.
+    bool IsRewriteDefaultArgument = false;
+
----------------
cor3ntin wrote:
> Can `IsRewriteDefaultArgument` and `MaterializePRValueInDiscardStatement` have different values?
> Maybe `MaterializePRValueInDiscardStatement` is sufficient
Currently they have the same value, but they mean different things, so I use two separated variables.


================
Comment at: clang/include/clang/Sema/Sema.h:1361
+    /// Eg. Extending the lifetime of temporaries in the init expression.
+    bool IsCheckingCXXForRangeInitVariable = false;
+
----------------
cor3ntin wrote:
> Either `IsInLifetimeExtendingContext` or `IsInCXXForRangeInitializer` seem like better names
Thanks for your suggestion, `IsInLifetimeExtendingContext ` looks good to me, I can use this var name later.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:6250
+            CallLoc, Param, CurContext};
       EnsureImmediateInvocationInDefaultArgs Immediate(*this);
       ExprResult Res;
----------------
@cor3ntin Does `EnsureImmediateInvocationInDefaultArgs` need to be renamed to a more generic name?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:18168
       Prev.InImmediateEscalatingFunctionContext;
-
   Cleanup.reset();
----------------
cor3ntin wrote:
> Whitespace only change
Thanks, I'll remove this.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:8206-8209
+    // We do not materialize temporay by default in order to avoid creating
+    // unnecessary temporary objects. If we skip this step, IR generation is
+    // able to synthesize the storage for itself in the aggregate case, and
+    // adding the extra node to the AST is just clutter.
----------------
cor3ntin wrote:
> Correct me if I'm wrong but the thing we want to avoid here is to avoid creating unnecessary AST nodes, right?
Yeah, ignore unnecessary AST nodes and runtime memory allocation. The redundant alloca/store instructions may be removed during the optimization phase.

Eg. 
```
static_cast<void>(42);
```
```
define noundef i32 @main() {
entry:
  %retval = alloca i32, align 4
  %ref.tmp = alloca i32, align 4
  store i32 0, ptr %retval, align 4
  store i32 42, ptr %ref.tmp, align 4
  call void @f()()
  ret i32 0
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701