[PATCH] D153701: [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 a comment.

@rsmith Thanks a lot for your comments and sorry for the very late reply, I was on vacation some time ago.

In D153701#4643609 <https://reviews.llvm.org/D153701#4643609>, @rsmith wrote:

> The changes in `SemaInit.cpp` don't look correct to me. Trying to recurse through the initializer and find every nested temporary is highly error-prone; I can't tell you which expression nodes you forgot to recurse through, but I'm sure there are some.
>
> I think the approach being taken here is not really maintainable. We don't want the initialization code to need to know how to recurse through an expression after the fact and find all the temporaries that might need to be lifetime-extended, and we don't need it to do that either. Instead, we should make the expression evaluation context track the current for-range variable, and in `Sema::CreateMaterializeTemporaryExpr`, we should create a temporary that is already set to be lifetime-extended by the loop variable.

I agree, I have reverted the changes in `SemaInit.cpp`. I have ever tried to do lifetime-extend in `Sema::CreateMaterializeTemporaryExpr`, but I fall into some trouble, the `MaterializeTemporaryExpr` was created before the for-range VarDecl, so we don't have a VarDecl for `MaterializeTemporaryExpr::setExtendingDecl` in  `Sema::CreateMaterializeTemporaryExpr`, I have a possible solution here, Eg. we allocate a memory block which has same size of VarDecl, and pass this pointer to `MaterializeTemporaryExpr::setExtendingDecl` as a placeholder VarDecl, when we build the for-range statement, we just construct a VarDecl in the memory block that we allocated before. But this approach doesn't look very good and not maintainable. So I use `ForRangeInitTemporaryLifetimeExtensionVisitor` to visit every temporaries in the initializer and extend the lifetime. And `ForRangeInitTemporaryLifetimeExtensionVisitor` was derived from `RecursiveASTVisitor`, maybe this approach be able to handle all temporaries. WDYT?



================
Comment at: clang/include/clang/Sema/Sema.h:1356-1357
 
+    /// Whether rewrite the default argument.
+    bool IsRewriteDefaultArgument = false;
+
----------------
rsmith wrote:
> Can you expand this comment to more fully describe what this flag governs? Which default argument? How would it be rewritten?
Yeah, this variable has been removed. By default, all CallExprs in Clang share the same CXXDefaultArgExpr from parameters, but in some contexts (such as lifetime extension), the lifetime of the temporaries in the current default parameters needs to be extended, so the CXXDefaultArgExpr needs to be copied and the lifetime of temporaries in the copy need to be extended. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701