Tyker added inline comments.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:11883-11885
ExprResult Result =
ActOnFinishFullExpr(Init, VDecl->getLocation(),
/*DiscardedValue*/ false, VDecl->isConstexpr());
----------------
rsmith wrote:
> This may create additional AST nodes outside the `ConstantExpr`, which
> `VarDecl::evaluateValue` is not expecting to see (in particular, if we have
> cleanups for the initializer). Should the `ConstantExpr` go outside those
> nodes rather than inside?
i removed the changes to the storage of constexpr values since it was only used
for testing purposes and we can now use consteval for that purpose.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:9635
+ if (IsExpr) {
+ Base = APValue::LValueBase(ReadExpr(F), CallIndex, Version);
+ ElemTy = Base.get<const Expr *>()->getType();
----------------
rsmith wrote:
> Tyker wrote:
> > Tyker wrote:
> > > rsmith wrote:
> > > > 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.
> > > Would it be possible to adapt serialization/deserialization so that they
> > > make sure that `MaterializeTemporaryExpr` are unique.
> > > by:
> > >
> > > - When serializing `MaterializeTemporaryExpr` serialize a key obtained
> > > from the pointer to the expression as it is unique.
> > > - When deserializing `MaterializeTemporaryExpr` deserializing the key,
> > > and than have a cache for previously deserialized expression that need to
> > > be unique.
> > >
> > > This would make easier adding new `Expr` that require uniqueness and seem
> > > less complicated.
> > > What do you think ?
> > i added a review that does the refactoring https://reviews.llvm.org/D69360.
> What are the cases for which we still encounter expressions as lvalue bases
> during serialization? I think all the other ones should be OK, but maybe
> there's another interesting one we've overlooked.
the 2 example that come to mind are string literals and type_info there are
probably more.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63640/new/
https://reviews.llvm.org/D63640
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
