Tyker marked 10 inline comments as done.
Tyker added a comment.
update done tasks.
================
Comment at: clang/lib/AST/APValue.cpp:599
Out << '[' << Path[I].getAsArrayIndex() << ']';
- ElemTy = Ctx.getAsArrayType(ElemTy)->getElementType();
+ ElemTy = cast<ArrayType>(ElemTy)->getElementType();
}
----------------
aaron.ballman wrote:
> Tyker wrote:
> > aaron.ballman wrote:
> > > Are you sure this doesn't change behavior? See the implementation of
> > > `ASTContext::getAsArrayType()`. Same question applies below.
> > i ran the test suite after the change it there wasn't any test failures.
> > but the test on dumping APValue are probably not as thorough as we would
> > like them to be.
> > from analysis of `ASTContext::getAsArrayType()` the only effect i see on
> > the element type is de-sugaring and canonicalization which shouldn't affect
> > correctness of the output. de-sugaring requires the ASTContext but
> > canonicalization doesn't.
> >
> > i think the best way the have higher confidence is to ask rsmith what he
> > thinks.
> Yeah, I doubt we have good test coverage for all the various behaviors here.
> I was wondering if the qualifiers bit was handled properly with a simple
> cast. @rsmith is a good person to weigh in.
the original question we had is whether it is correct to replace
`Ctx.ASTContext::getAsArrayType(ElemTy)` by
`cast<ArrayType>(ElemTy.getCanonicalType())` in this context and the other
comment below.
================
Comment at: clang/lib/AST/Expr.cpp:319
case RSK_None:
return;
case RSK_Int64:
----------------
rsmith wrote:
> 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`?)
we can put `llvm_unreachable` in the switch because of `if (!Value.hasValue())`
above the switch but we can't remove `if (!Value.hasValue())`.
all cases i have seen where `if (!Value.hasValue())` is taken occur after a
semantic error occured.
================
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:
> 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 ?
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
