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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:120
 
-  // FIXME: Add `StorageLocation` for `this`.
+  StorageLocation *ThisPointeeLoc = nullptr;
 
----------------
It feels a bit wrong to have a separate `ThisPointeeLoc` here. But as far as I understand, this is an artifact of the AST representation we have. I think a superior AST representation would have an implicit `ParmVarDecl` to represent `this`, so we would not need to do any special handling at all, the general code path would do the right thing. I think the C++ language, with the `deducing this` started its approach in this direction officially. I really hope someone will have the time to fix all the mess and get rid of all the unnecessary corner cases in the foreseeable future :) 


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:52
+      setStorageLocation(*ParamDecl, ParamLoc);
+      initValueInStorageLocation(ParamLoc, ParamDecl->getType());
+    }
----------------
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.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:131
+
+    auto &Loc = Env.createStorageLocation(*S);
+    Env.setStorageLocation(*S, Loc);
----------------
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.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:141-142
+
+    if (Member->isFunctionOrFunctionTemplate())
+      return;
+
----------------
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. 


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:813
+
+    void target(A &Foo) {
+      (void)0;
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117012