rsmith added a comment.
Thank you, this looks like a great direction.
As noted, there are a bunch of other cases that we should cover with this
approach. I'm really happy about the number of related bugs we get to fix with
this change.
================
Comment at: lib/AST/ExprConstant.cpp:3186-3187
// object under construction.
- if (Info.isEvaluatingConstructor(LVal.getLValueBase(), LVal.CallIndex)) {
+ if (Info.isEvaluatingConstructor(LVal.getLValueBase(),
+ LVal.getLValueCallIndex())) {
BaseType = Info.Ctx.getCanonicalType(BaseType);
----------------
This should take the version into account.
================
Comment at: lib/AST/ExprConstant.cpp:5236
if (Frame) {
- Result.set(VD, Frame->Index);
+ Result.set({VD, Frame->Index});
return true;
----------------
Hmm. We should be versioning local variables as well. Currently we'll accept
invalid code such as:
```
constexpr int f() {
int *p = nullptr;
for (int k = 0; k != 2; ++k) {
int local_var = 0;
if (k == 0)
p = &local_var;
else
return *p;
}
}
static_assert(f() == 0);
```
================
Comment at: lib/AST/ExprConstant.cpp:5275-5278
+ unsigned Version = Info.CurrentCall->getMTEVersion();
Value = &Info.CurrentCall->
- createTemporary(E, E->getStorageDuration() == SD_Automatic);
- Result.set(E, Info.CurrentCall->Index);
+ createTemporary(E, E->getStorageDuration() == SD_Automatic, Version);
+ Result.set({E, Info.CurrentCall->Index, Version});
----------------
Can you combine these, so we have a single function to create a temporary and
produce both an `LValue` denoting it and an `APValue*` to hold its evaluated
value?
================
Comment at: lib/AST/ExprConstant.cpp:5772
} else {
- Result.set(SubExpr, Info.CurrentCall->Index);
+ Result.set({SubExpr, Info.CurrentCall->Index});
if (!EvaluateInPlace(Info.CurrentCall->createTemporary(SubExpr, false),
----------------
This should create a versioned temporary object.
================
Comment at: lib/AST/ExprConstant.cpp:6540
bool VisitConstructExpr(const Expr *E) {
- Result.set(E, Info.CurrentCall->Index);
+ Result.set({E, Info.CurrentCall->Index});
return EvaluateInPlace(Info.CurrentCall->createTemporary(E, false),
----------------
This should create a versioned temporary object.
================
Comment at: lib/AST/ExprConstant.cpp:8031
+ (A.getLValueCallIndex() == B.getLValueCallIndex() &&
+ A.getLValueVersion() == B.getLValueVersion());
}
----------------
You already checked this above. It'd make sense to check the call index and
version in the same place here, but we should only need one check for each :)
================
Comment at: lib/AST/ExprConstant.cpp:9974-9997
+ LV.set({E, Info.CurrentCall->Index});
APValue &Value = Info.CurrentCall->createTemporary(E, false);
if (!EvaluateArray(E, LV, Value, Info))
return false;
Result = Value;
} else if (T->isRecordType()) {
LValue LV;
----------------
These temporaries should all be versioned.
https://reviews.llvm.org/D42776
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
