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

yronglin added a comment. In D153701#4656920 , @Endill wrote: > @yronglin We are sorry it takes so much time to get feedback. Richard and > Hubert have little bandwidth for reviews. Others, including me, don't feel > qualified to provide good feedback.

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

Endill added a comment. @yronglin We are sorry it takes so much time to get feedback. Richard and Hubert have little bandwidth for reviews. Others, including me, don't feel qualified to provide good feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

yronglin added a comment. ping 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

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

yronglin added a comment. friendly ping~ 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

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

yronglin added a comment. ping～ 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

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

yronglin marked an inline comment as done. yronglin added a comment. ping~ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153701/new/ https://reviews.llvm.org/D153701 ___ cfe-commits mailing list

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

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 , @rsmith wrote: > The changes in `SemaInit.cpp` don't

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

yronglin updated this revision to Diff 557705. yronglin added a comment. Update 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

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

rsmith added a comment. 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

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

yronglin added a comment. ping～ 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

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

yronglin updated this revision to Diff 556035. yronglin added a comment. Rebase 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

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

yronglin added a comment. ping~ 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

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

yronglin added a comment. friendly ping~ 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

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

yronglin added a comment. ping~ @hubert.reinterpretcast @rsmith Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153701/new/ https://reviews.llvm.org/D153701 ___ cfe-commits mailing list

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

yronglin updated this revision to Diff 552009. yronglin marked 7 inline comments as done. yronglin added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153701/new/ https://reviews.llvm.org/D153701 Files:

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

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:

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

cor3ntin added a comment. I'm really not qualified to give you meaningful feedback on design here, but I spotted a few minor things that can be improved, I hope it helps! Comment at: clang/include/clang/Sema/Sema.h:1357 +/// Whether rewrite the default argument. +bool

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

yronglin updated this revision to Diff 551871. yronglin added a comment. Do not spass MaterializePRValueInDiscardStatement in PushExpressionEvaluationContext. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153701/new/

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

yronglin updated this revision to Diff 551851. yronglin added a comment. Fix ci 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

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

yronglin updated this revision to Diff 551845. yronglin added a comment. Fix ci failure, and introduce an variable in ExpressionEvaluationContextRecord to rewrite default argument. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153701/new/

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

yronglin added a comment. friendly ping~ 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

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

yronglin added a comment. In D153701#4563919 , @yronglin wrote: > 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

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

yronglin added a comment. @cor3ntin I have reused `EnsureImmediateInvocationInDefaultArgs` to rewrite `CXXDefaultArgExpr`, does this is a correct approach? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153701/new/ https://reviews.llvm.org/D153701

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

yronglin updated this revision to Diff 549896. yronglin added a comment. Add test to check generated LLVM IR, and fix crash. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153701/new/ https://reviews.llvm.org/D153701 Files:

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

yronglin added a comment. Should we dump the real default argument AST but not only `CXXDefaultArgExpr` if `CXXDefaultArgExpr` has been rewritted? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153701/new/ https://reviews.llvm.org/D153701

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

yronglin added a comment. FIXME: Need add test in `clang/test/CXX/special/class.temporary/p6.cpp` to check generated LLVM IR. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153701/new/ https://reviews.llvm.org/D153701

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

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

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

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

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

yronglin added a comment. In D153701#4459036 , @cor3ntin wrote: > Sorry, I missed the ping on Discord. > Thanks for working on this > > I don't feel qualified to review this, but I don't think there are nearly > enough tests. > FYI there is a previous

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

yronglin added a comment. In D153701#4459036 , @cor3ntin wrote: > Sorry, I missed the ping on Discord. > Thanks for working on this > > I don't feel qualified to review this, but I don't think there are nearly > enough tests. > FYI there is a previous

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

cor3ntin added a comment. Sorry, I missed the ping on Discord. Thanks for working on this I don't feel qualified to review this, but I don't think there are nearly enough tests. FYI there is a previous attempt at this here https://reviews.llvm.org/D139586 - with some test suggestions

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

yronglin updated this revision to Diff 535414. yronglin added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153701/new/ https://reviews.llvm.org/D153701 Files: clang/docs/ReleaseNotes.rst clang/include/clang/AST/Decl.h

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

yronglin added a comment. friendly ping~ 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

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

yronglin updated this revision to Diff 534920. yronglin added a comment. Poke CI Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153701/new/ https://reviews.llvm.org/D153701 Files: clang/docs/ReleaseNotes.rst clang/include/clang/AST/Decl.h

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

yronglin updated this revision to Diff 534610. yronglin added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153701/new/ https://reviews.llvm.org/D153701 Files: clang/docs/ReleaseNotes.rst clang/include/clang/AST/Decl.h

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

yronglin updated this revision to Diff 534559. yronglin added a comment. Format and rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153701/new/ https://reviews.llvm.org/D153701 Files: clang/docs/ReleaseNotes.rst

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

yronglin updated this revision to Diff 534212. yronglin added a comment. Update c++ status Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153701/new/ https://reviews.llvm.org/D153701 Files: clang/docs/ReleaseNotes.rst

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

yronglin created this revision. Herald added a project: All. yronglin requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Signed-off-by: yronglin Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D153701 Files: