[PATCH] D76287: [analysis][analyzer] Introduce the skeleton of a reaching definitions calculator

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
Comment at: clang/include/clang/Analysis/Analyses/ReachingDefinitions.h:79
+
+struct VarAndCFGElementLess {
+  bool operator()(Definition Lhs, Definition Rhs) const {
----------------
Shouldn't this be called `DefinitionLess` if this is the "natural" comparator for `Definition`? Also, is this the convention in LLVM, instead of providing an explicit specialisation for `std::less`?


================
Comment at: clang/include/clang/Analysis/Analyses/ReachingDefinitions.h:131
+
+  // Fields that shouldn't change after the construction of the builder object.
+
----------------
I do not feel this is visually separative enough, the fact that there is a free-floating comment (seemingly bogusly not attached to any declaration) breaks reading of the code.

How about this:


```
class X {
  // Fields that should not change after construction
  private:
    Foo bar;
    Bla blah;

  // Fields that change at every step
  private:
     Blah bla;

  public:
    Yadda blebba;
};
```


================
Comment at: clang/include/clang/Analysis/Analyses/ReachingDefinitions.h:140
+  const Decl *D;
+  ASTContext *Context = nullptr;
+
----------------
You do not seem to be using this variable but in one place. Are you sure it is worth the saving as a field? Also, is it valid in the first place to have `this->Context` not to be the same as `this->D->getASTContext()`?


================
Comment at: clang/include/clang/Analysis/Analyses/ReachingDefinitions.h:176-179
+  //===--------------------------------------------------------------------===//
+  // Utility methods for building a GEN set. These are public because the
+  // callback objects will need to call them.
+  //===--------------------------------------------------------------------===//
----------------
Is this the right approach? A public method makes the user itch to call it. If `GenSetMatcherCallback` is a superclass of every possible implementation, I think adding that class as a friend here works, and you could make the methods private, or protected.


================
Comment at: clang/include/clang/Analysis/Analyses/ReachingDefinitions.h:236-242
+  // TODO: Should we change to ImmutableMap? Does this matter that much?
+  std::map<const CFGBlock *, GenSet> Gen;
+  std::map<const CFGBlock *, DefinitionSet> Kill;
+
+public:
+  std::map<const CFGBlock *, DefinitionSet> In;
+  std::map<const CFGBlock *, DefinitionSet> Out;
----------------
The immutability might not(?), but the performance aspects of the RBT behind STL `map` could? `K` is a pointer, why not use `DenseMap`? How large do you expect these containers to be when they peak out?


================
Comment at: clang/lib/Analysis/ReachingDefinitions.cpp:96
+  static internal::Matcher<Stmt> getMatcher() {
+    return binaryOperator(isAssignmentOperator()).bind("assign");
+  }
----------------
I know this is the first patch, but it might be worth mentioning here that this thing does not match user-defined operators.


================
Comment at: clang/lib/Analysis/ReachingDefinitions.cpp:162
+
+  // TODO: Destructor calls? Should we be THAT conservative?
+  // TODO: Regular function calls?
----------------
You mean //explicit// destructor calls here, right?


================
Comment at: clang/lib/Analysis/ReachingDefinitions.cpp:275
+void ReachingDefinitionsCalculator::dumpGenSet() const {
+  llvm::errs() << "GEN sets: blockid (varname [blockid, elementid])\n";
+  for (const std::pair<const CFGBlock *, GenSet> D : Gen) {
----------------
Instead of simply blockID, could you harmonise this output with the CFG dump and say `Bx` instead of just `X`?


================
Comment at: clang/test/Analysis/dump-definitions.cpp:17-18
+
+// CHECK:      GEN sets: blockid (varname [blockid, elementid])
+// CHECK-NEXT: 1 (ptr, [1, 3]) <write>
+// CHECK-NEXT: KILL sets: blockid (varname [blockid, elementid])
----------------
What is an //element//? How do they get their numbers? What does the `3` mean here? I get that basic block 1 (the body of the function) writes `ptr`... but I don't understand this further from looking at the expected output.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76287