[PATCH] D34329: [GSoC] Clang AST diffing

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

Congratulations of the first GSoC patch! I have some below comments:

- Patches should be submitted using the full context (`git diff -U9999`). This makes it easier for reviewers to understand the change. This patch mainly adds new code, so this won't make much difference, but please keep this in mind as you update this patch and submit new patches.



================
Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:11
+///
+/// \file
+/// \brief
----------------
Missing doc comments? You could provide a brief, high-level overview of the algorithm that you're implementing.


================
Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:61
+public:
+  ASTUnit *
+  std::vector<NodeId> Leaves;
----------------
What do you use from the `ASTUnit` in the code? Is it possible to use `ASTContext` instead?


================
Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:118
+
+} // namespace diff
+} // namespace clang
----------------
Use `end namespace diff` instead. This applies to any other closing namespace comments as well.


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:31
+
+static bool isRelevantNode(Decl *D) { return D != nullptr; }
+static bool isRelevantNode(Stmt *S) { return S != nullptr; }
----------------
I don't see the point in these functions. Are you going to add some more logic to them?

I'd suggest removing them and using early return in the traversal functions instead, e.g:

```
  bool TraverseDecl(Decl *D) {
    if (!D)
      return true;
    ++Count;
    return RecursiveASTVisitor<NodeCountVisitor>::TraverseDecl(D);
  }
```


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:187
+  if (auto *X = DTN.get<StringLiteral>()) {
+    return X->getString();
+  }
----------------
You don't need the braces around this `if` and the ones below (This suggestion probably applies to a few other places as well).


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:239
+  }
+  outs() << formatv(R"({"typeLabel":"{0}"{1},"children":[)", N.getTypeLabel(),
+                    Label);
----------------
Typically `dump` methods print to `stderr`, I would suggest renaming these methods to `print`. You should also pass-in a `raw_ostream &` to these methods to allow output to different streams. And then you can provide a `printAsJson` implementation without any parameters that just passes `llvm::outs()` to the `printAsJson` that takes in the `raw_ostream &`.

Similar comments apply to other `dump` methods.


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:351
+      }
+      assert(K >= 0);
+      KeyRoots[K] = I;
----------------
`assert`s should typically have a message describing the failure, e.g.:

`assert(K >= 0 && "Expected a positive non-zero K");`


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:439
+    for (int I = 0; I < S1.getSizeS() + 1; ++I) {
+      delete[] TreeDist[I];
+      delete[] ForestDist[I];
----------------
You can use `std::unique_ptr<std::unique_ptr<double []> []>` and avoid the custom destructor. The constructor will then be something like:

```
TreeDist = llvm::make_unique<std::unique_ptr<double []> []>(S1.getSizeS() + 1);
ForestDist = llvm::make_unique<std::unique_ptr<double []> []>(S1.getSizeS() + 1);
for (int I = 0, E = S1.getSizeS() + 1; I < E; ++I) {
  TreeDist[I] = llvm::make_unique<double []>(S2.getSizeS() + 1);
  ForestDist[I] = llvm::make_unique<double []>(S2.getSizeS() + 1);
}
```


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:508
+};
+} // namespace
+
----------------
Use `// end anonymous namespace` instead


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:814
+  double MinDice = 0.2;
+  int MaxSize = 100;
+
----------------
Please document these parameters and their default values.


================
Comment at: tools/clang-diff/ClangDiff.cpp:10
+///
+/// \file
+/// \brief
----------------
Missing file comment. You should either delete this or add some description.


https://reviews.llvm.org/D34329