[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

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

rsmith added inline comments.


================
Comment at: clang/lib/AST/Expr.cpp:319
   case RSK_None:
     return;
   case RSK_Int64:
----------------
Can you use `llvm_unreachable` here? (Are there cases where we use `RSK_None` and then later find we actually have a value to store into the `ConstantExpr`?)


================
Comment at: clang/lib/Serialization/ASTReader.cpp:9635
+        if (IsExpr) {
+          Base = APValue::LValueBase(ReadExpr(F), CallIndex, Version);
+          ElemTy = Base.get<const Expr *>()->getType();
----------------
This is problematic.

`ReadExpr` will read a new copy of the expression, creating a distinct object. But in the case where we reach this when deserializing (for a `MaterializeTemporaryExpr`), we need to refer to the existing `MaterializeTemporaryExpr` in the initializer of its lifetime-extending declaration. We will also need to serialize the `ASTContext`'s `MaterializedTemporaryValues` collection so that the temporaries lifetime-extended in a constant initializer get properly handled.

That all sounds very messy, so I think we should reconsider the model that we use for lifetime-extended materialized temporaries. As a half-baked idea:

 * When we lifetime-extend a temporary, create a `MaterializedTemporaryDecl` to hold its value, and modify `MaterializeTemporaryExpr` to refer to the `MaterializedTemporaryDecl` rather than to just hold the subexpression for the temporary.
 * Change the `LValueBase` representation to denote the declaration rather than the expression.
 * Store the constant evaluated value for a materialized temporary on the `MaterializedTemporaryDecl` rather than on a side-table in the `ASTContext`.

With that done, we should verify that all remaining `Expr*`s used as `LValueBase`s are either only transiently used during evaluation or don't have these kinds of identity problems.


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

https://reviews.llvm.org/D63640