[PATCH] D117012: [clang][dataflow] Add transfer functions for data members and this pointers

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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