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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added a comment.

Thank you both for the reviews!



================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:52
+      setStorageLocation(*ParamDecl, ParamLoc);
+      initValueInStorageLocation(ParamLoc, ParamDecl->getType());
+    }
----------------
xazax.hun wrote:
> There might be an optimization opportunity for `initValueInStorageLocation`. If we initialize a class or struct, we will end up create locations for all of the fields recursively (unless self-referential) right? Actually, in many of the codebases we cannot even access many of those fields because they might be private/protected etc. Unfortunately, it might not be easy to query whether a field is accessible from this method, but I wonder if it is worth a TODO somewhere.
Right. I added a FIXME in `initValueInStorageLocationUnlessSelfReferential`.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:57
+  if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(&DeclCtx)) {
+    if (!MethodDecl->isStatic()) {
+      QualType ThisPointeeType = MethodDecl->getThisObjectType();
----------------
ymandel wrote:
> nit: invert the if and return?
I prefer to keep the block self-contained as we might end up adding code after the outer if statement.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:131
+
+    auto &Loc = Env.createStorageLocation(*S);
+    Env.setStorageLocation(*S, Loc);
----------------
xazax.hun wrote:
> Nit: I got confused for a second what will happen in a loop. I wonder if `createStorageLocation` is better renamed to `createOrGetStorageLocation` to express the fact it will not always create a new location. But I don't have strong feelings about this, also feel free to defer to a later PR.
Ack. I added a FIXME.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:141-142
+
+    if (Member->isFunctionOrFunctionTemplate())
+      return;
+
----------------
xazax.hun wrote:
> I wonder if we also want to create a non-null pointer value for these in the future so we can evaluate certain if statements. 
I added a FIXME.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:813
+
+    void target(A &Foo) {
+      (void)0;
----------------
xazax.hun wrote:
> I wonder if we can make the tests a bit more concise by merging some of them. E.g. we could have a single test with both a pointer, a reference, and a value param. Although I understand that some people like to keep most tests minimal, so feel free to ignore this.
I'll keep the tests separate for now, but I'll think about how to make them more concise.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:919
+
+        EXPECT_EQ(Env.getValue(*BazDecl, SkipPast::None), BarVal);
+      });
----------------
ymandel wrote:
> Why EXPECT here but ASSERT (as last line) in previous tests?
My bad. I updated all tests to use ASSERT only where necessary.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1059
+
+TEST_F(TransferTest, ClassThisMember) {
+  std::string Code = R"(
----------------
ymandel wrote:
> 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?
For completeness. Changing the `isStructureOrClassType` predicate in `initValueInStorageLocationUnlessSelfReferential` to `isStructureType` or `isClassType` should make one of these tests fail. I'll address public and private members in a follow up.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1062
+    class A {
+      int Bar;
+
----------------
xazax.hun wrote:
> I'd love to see a test with multiple fields and a nested struct.
I added a nested struct and an extra member.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117012/new/

https://reviews.llvm.org/D117012