ymandel updated this revision to Diff 486250.
ymandel added a comment.
add clarifying comment regarding interpration of `DecompositionDecl`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140897/new/
https://reviews.llvm.org/D140897
Files:
clang/lib/Analysis/FlowSensitive/Transfer.cpp
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3739,6 +3739,8 @@
const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz");
ASSERT_THAT(BazDecl, NotNull());
+ // BindingDecls always map to references -- either lvalue or rvalue, so
+ // we still need to skip here.
const Value *BoundFooValue =
Env1.getValue(*BoundFooDecl, SkipPast::Reference);
ASSERT_THAT(BoundFooValue, NotNull());
@@ -3750,13 +3752,13 @@
EXPECT_TRUE(isa<IntegerValue>(BoundBarValue));
// Test that a `DeclRefExpr` to a `BindingDecl` works as expected.
- EXPECT_EQ(Env1.getValue(*BazDecl, SkipPast::Reference), BoundFooValue);
+ EXPECT_EQ(Env1.getValue(*BazDecl, SkipPast::None), BoundFooValue);
const Environment &Env2 = getEnvironmentAtAnnotation(Results, "p2");
// Test that `BoundFooDecl` retains the value we expect, after the join.
BoundFooValue = Env2.getValue(*BoundFooDecl, SkipPast::Reference);
- EXPECT_EQ(Env2.getValue(*BazDecl, SkipPast::Reference), BoundFooValue);
+ EXPECT_EQ(Env2.getValue(*BazDecl, SkipPast::None), BoundFooValue);
});
}
@@ -3834,16 +3836,15 @@
// works as expected. We don't test aliasing properties of the
// reference, because we don't model `std::get` and so have no way to
// equate separate references into the tuple.
- EXPECT_EQ(Env1.getValue(*BazDecl, SkipPast::Reference), BoundFooValue);
+ EXPECT_EQ(Env1.getValue(*BazDecl, SkipPast::None), BoundFooValue);
const Environment &Env2 = getEnvironmentAtAnnotation(Results, "p2");
// Test that `BoundFooDecl` retains the value we expect, after the join.
BoundFooValue = Env2.getValue(*BoundFooDecl, SkipPast::Reference);
- EXPECT_EQ(Env2.getValue(*BazDecl, SkipPast::Reference), BoundFooValue);
+ EXPECT_EQ(Env2.getValue(*BazDecl, SkipPast::None), BoundFooValue);
});
}
-// TODO: ref binding
TEST(TransferTest, BinaryOperatorComma) {
std::string Code = R"(
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -108,7 +108,8 @@
}
// Unpacks the value (if any) associated with `E` and updates `E` to the new
-// value, if any unpacking occured.
+// value, if any unpacking occured. Also, does the lvalue-to-rvalue conversion,
+// by skipping past the reference.
static Value *maybeUnpackLValueExpr(const Expr &E, Environment &Env) {
auto *Loc = Env.getStorageLocation(E, SkipPast::Reference);
if (Loc == nullptr)
@@ -145,7 +146,9 @@
if (LHSLoc == nullptr)
break;
- auto *RHSVal = Env.getValue(*RHS, SkipPast::Reference);
+ // No skipping should be necessary, because any lvalues should have
+ // already been stripped off in evaluating the LValueToRValue cast.
+ auto *RHSVal = Env.getValue(*RHS, SkipPast::None);
if (RHSVal == nullptr)
break;
@@ -195,7 +198,15 @@
if (DeclLoc == nullptr)
return;
- if (VD->getType()->isReferenceType()) {
+ // If the value is already an lvalue, don't double-wrap it.
+ if (isa_and_nonnull<ReferenceValue>(Env.getValue(*DeclLoc))) {
+ // We only expect to encounter a `ReferenceValue` for a reference type
+ // (always) or for `BindingDecl` (sometimes). For the latter, we can't
+ // rely on type, because their type does not indicate whether they are a
+ // reference type. The assert is not strictly necessary, since we don't
+ // depend on its truth to proceed. But, it verifies our assumptions,
+ // which, if violated, probably indicate a problem elsewhere.
+ assert(VD->getType()->isReferenceType() || isa<BindingDecl>(VD));
Env.setStorageLocation(*S, *DeclLoc);
} else {
auto &Loc = Env.createStorageLocation(*S);
@@ -235,6 +246,7 @@
if (D.getType()->isReferenceType()) {
// Initializing a reference variable - do not create a reference to
// reference.
+ // FIXME: reuse the ReferenceValue instead of creating a new one.
if (auto *InitExprLoc =
Env.getStorageLocation(*InitExpr, SkipPast::Reference)) {
auto &Val =
@@ -261,6 +273,9 @@
Env.setValue(Loc, *Val);
}
+ // `DecompositionDecl` must be handled after we've interpreted the loc
+ // itself, because the binding expression refers back to the
+ // `DecompositionDecl` (even though it has no written name).
if (const auto *Decomp = dyn_cast<DecompositionDecl>(&D)) {
// If VarDecl is a DecompositionDecl, evaluate each of its bindings. This
// needs to be evaluated after initializing the values in the storage for
@@ -285,7 +300,8 @@
// types. The holding var declarations appear *after* this statement,
// so we have to create a location or them here to share with `B`. We
// don't visit the binding, because we know it will be a DeclRefExpr
- // to `VD`.
+ // to `VD`. Note that, by construction of the AST, `VD` will always be
+ // a reference -- either lvalue or rvalue.
auto &VDLoc = Env.createStorageLocation(*VD);
Env.setStorageLocation(*VD, VDLoc);
Env.setStorageLocation(*B, VDLoc);
@@ -317,7 +333,8 @@
case CK_LValueToRValue: {
// When an L-value is used as an R-value, it may result in sharing, so we
- // need to unpack any nested `Top`s.
+ // need to unpack any nested `Top`s. We also need to strip off the
+ // `ReferenceValue` associated with the lvalue.
auto *SubExprVal = maybeUnpackLValueExpr(*SubExpr, Env);
if (SubExprVal == nullptr)
break;
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
