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