ymandel accepted this revision.
ymandel added a comment.
This revision is now accepted and ready to land.
Thanks!
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:57
+ if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(&DeclCtx)) {
+ if (!MethodDecl->isStatic()) {
+ QualType ThisPointeeType = MethodDecl->getThisObjectType();
----------------
nit: invert the if and return?
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:145
+ auto *BaseLoc = cast_or_null<AggregateStorageLocation>(
+ Env.getStorageLocation(*S->getBase(), SkipPast::ReferenceThenPointer));
+ if (BaseLoc == nullptr)
----------------
nit: maybe comment to explain why we use this setting?
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:834
+ const ReferenceValue *FooVal =
+ cast<ReferenceValue>(Env.getValue(*FooLoc));
+ const StorageLocation &FooPointeeLoc = FooVal->getPointeeLoc();
----------------
maybe dyn_cast with ASSERT? (or somesuch)
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:919
+
+ EXPECT_EQ(Env.getValue(*BazDecl, SkipPast::None), BarVal);
+ });
----------------
Why EXPECT here but ASSERT (as last line) in previous tests?
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1040
+ const auto *ThisLoc =
+
cast<AggregateStorageLocation>(Env.getThisPointeeStorageLocation());
+
----------------
dyn_cast and ASSERT (or somesuch)? I'd think that whether or not we have a
registered `this` in the environment is something we want to test?
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1059
+
+TEST_F(TransferTest, ClassThisMember) {
+ std::string Code = R"(
----------------
why test a class distinct from a struct? Is there a meaningful difference
between the two with regard to our model? if so, might it be worth instead
testing explicit public vs private?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D117012/new/
https://reviews.llvm.org/D117012
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
