[PATCH] D69360: [NFC] Refactor representation of materialized temporaries

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks, I like this a lot.

There are a few more changes we'll need in addition to this before we can properly serialize `APValue`s referring to such temporaries:

1. Merging during deserialization. For example, if we have

  inline int &&r = 123;

in two different modules, we should merge the two `LifetimeExtendedTemporaryDecl`s for the `123`. This will require making the decl derive from `Mergeable<...>` and adding a little logic to ASTReader to look up lifetime-extended declarations by their extending decl and mangling number.

2. Change the constant expression representation for a pointer or reference for a materialized temporary to refer to the temporary declaration instead.

3. Change CodeGen / name mangling to operate on the new declaration node rather than `MaterializedTemporaryExpr`. (This will be needed once `CodeGen` starts seeing constant `APValue`s referring to the new declaration nodes.)

I'm happy for the above to be done in a follow-up; as is, this change is a good refactoring and a basis for the above extensions.



================
Comment at: clang/include/clang/AST/DeclCXX.h:3057
+/// MaterializeTemporaryExpr
+class MaterializeTemporaryDecl final : public Decl {
+  friend class MaterializeTemporaryExpr;
----------------
Since we should only use this for lifetime-extended temporaries, not for any materialized temporary, how about renaming to `LifetimeExtendedTemporaryDecl`?


================
Comment at: clang/include/clang/AST/DeclCXX.h:3071
+
+  mutable APValue* Value = nullptr;
+
----------------
Clang coding convention puts the `*` on the right (please fix throughout the patch).


================
Comment at: clang/include/clang/AST/DeclCXX.h:3101-3106
+  Stmt* getStmtWithTemporary() {
+    return StmtWithTemporary;
+  }
+  const Stmt* getStmtWithTemporary() const {
+    return StmtWithTemporary;
+  }
----------------
We should expose this as an `Expr*` rather than a `Stmt*`, since it must always be an expression. Also maybe `getSubExpr()` or `getTemporaryExpr()` would be a better name?

Please include a comment here saying that this isn't necessarily the initializer of the temporary due to the C++98 delayed materialization rules, but that `skipRValueSubobjectAdjustments` can be used to find said initializer within the subexpression.


================
Comment at: clang/include/clang/AST/ExprCXX.h:4469-4471
+  /// Get the storage for the constant value of a materialized temporary
+  /// of static storage duration.
+  APValue *getOrCreateValue(ASTContext &Ctx, bool MayCreate) const;
----------------
This should live on the declaration, not here.


================
Comment at: clang/include/clang/AST/ExprCXX.h:4516
+    auto ES = State.get<MaterializeTemporaryDecl *>();
+    return child_range(&ES->StmtWithTemporary, &ES->StmtWithTemporary + 1);
   }
----------------
Instead of the friend decl / direct member access, it would be cleaner to a `children` function to the declaration and call it from here.


================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2125
+DEF_TRAVERSE_DECL(MaterializeTemporaryDecl, {
+  TRY_TO(TraverseDecl(D->getExtendingDecl()));
+  TRY_TO(TraverseStmt(D->getStmtWithTemporary()));
----------------
We should not traverse the extending declaration here; the temporary just has a backreference to the extending decl, it doesn't *contain* the extending decl.

Also, this is unreachable in normal circumstances. I think traversing the `MaterializeTemporaryExpr` should visit the lifetime-extended temporary declaration if there is one.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69360