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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
