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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
