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