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

[Corentin Jabot via Phabricator via cfe-commits]

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 IsRewriteDefaultArgument = false;
+
----------------
Can `IsRewriteDefaultArgument` and `MaterializePRValueInDiscardStatement` have different values?
Maybe `MaterializePRValueInDiscardStatement` is sufficient


================
Comment at: clang/include/clang/Sema/Sema.h:1359-1361
+    /// Whether we are currently checking C++ for-range-init variable.
+    /// Eg. Extending the lifetime of temporaries in the init expression.
+    bool IsCheckingCXXForRangeInitVariable = false;
----------------



================
Comment at: clang/include/clang/Sema/Sema.h:1361
+    /// Eg. Extending the lifetime of temporaries in the init expression.
+    bool IsCheckingCXXForRangeInitVariable = false;
+
----------------
Either `IsInLifetimeExtendingContext` or `IsInCXXForRangeInitializer` seem like better names


================
Comment at: clang/include/clang/Sema/Sema.h:1363-1372
+    /// Whether we should materialize temporary the non `cv void` prvalue in
+    /// discard statement.
+    ///
+    /// [6.7.7.2.6] when a prvalue that has type other than cv void appears as a
+    /// discarded-value expression ([expr.context]).
+    ///
+    /// We do not materialize temporay by default in order to avoid creating
----------------



================
Comment at: clang/include/clang/Sema/Sema.h:9866
+
+  bool isMaterializePRValueInDiscardStatement() const {
+    assert(!ExprEvalContexts.empty() &&
----------------



================
Comment at: clang/lib/Parse/ParseDecl.cpp:2330-2333
+          getLangOpts().CPlusPlus23);
+
+      // P2718R0 - Lifetime extension in range-based for loops.
+      if (getLangOpts().CPlusPlus23) {
----------------
We need to decide whether we want to backport thgis behavior to older language mode, i think that would make sense.
@hubert.reinterpretcast wdyt?


================
Comment at: clang/lib/Parse/ParseDecl.cpp:2340-2344
+        // Materialize non-`cv void` prvalue temporaries in discard statement
+        // during parsing. These materialized temporaries may be extented
+        // lifetime.
+        LastRecord.MaterializePRValueInDiscardStatement = true;
+      }
----------------
A discarded statement is the non-taken branch of an if constexpr, so using `discarded expression` (or discarded value expression) will lead to less confusion.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:18168
       Prev.InImmediateEscalatingFunctionContext;
-
   Cleanup.reset();
----------------
Whitespace only change


================
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.
----------------
Correct me if I'm wrong but the thing we want to avoid here is to avoid creating unnecessary AST nodes, right?


================
Comment at: clang/lib/Sema/SemaInit.cpp:7660
+
+    if (auto *DAE = dyn_cast<CXXDefaultArgExpr>(Init)) {
+      Path.push_back(
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153701