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

Mon Sep 11 14:11:21 PDT 2023

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

================
Comment at: clang/include/clang/Sema/Sema.h:1356-1357

+    /// Whether rewrite the default argument.
+    bool IsRewriteDefaultArgument = false;
+
----------------
Can you expand this comment to more fully describe what this flag governs? Which default argument? How would it be rewritten?

================
Comment at: clang/lib/Parse/ParseDecl.cpp:2330-2333
+          getLangOpts().CPlusPlus23);
+
+      // P2718R0 - Lifetime extension in range-based for loops.
+      if (getLangOpts().CPlusPlus23) {
----------------
cor3ntin wrote:
> We need to decide whether we want to backport thgis behavior to older language mode, i think that would make sense.
> @hubert.reinterpretcast wdyt?
Definitely not. This is a visible behavior change to a rule that was not in any way incorrect or broken, and for which there were no implementation concerns. The change was not voted into the standard as a DR (see https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/n4929.html). Applying this behavior in older standard versions would be a conformance bug.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701