[PATCH] D34329: [GSoC] Clang AST diffing

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:156
+int TreeRoot::getSubtreePostorder(std::vector<NodeId> &Ids, NodeId Root) const {
+  int Leaves = 0;
+  std::function<void(NodeId)> Traverse = [&](NodeId Id) {
----------------
Please use a more descriptive name e.g. `NumLeaves`.


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:157
+  int Leaves = 0;
+  std::function<void(NodeId)> Traverse = [&](NodeId Id) {
+    const Node &N = getNode(Id);
----------------
you should be able to use `auto` instead of `std::function` here I think.


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:209
+    if (X->hasQualifier() && X->getQualifier()->getAsIdentifier())
+      Value += std::string(X->getQualifier()->getAsIdentifier()->getName());
+    Value += X->getDecl()->getNameAsString();
----------------
Qualifiers (i.e. `NestedNameSpecifier`s) can contain multiple identifiers (e.g. `foo::bar::`). You should use the `print` method from `NestedNameSpecifier` instead.


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:226
+  if (auto *X = DTN.get<Stmt>())
+    return Value;
+  if (auto *X = DTN.get<QualType>())
----------------
This `Value` will always be `""`, right? You can just return `""` here then.


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:755
+  size_t Position;
+  std::tie(Kind, Id1, Id2, Position) = Chg;
+  std::string S;
----------------
Looking at the unpacking here it's probably better to make `Change` a struct with named fields instead of a tuple. You would still be able to use `emplace` to create the changes though.


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:759
+  case Delete:
+    S = formatv("Delete {0}", T1.showNode(Id1));
+    break;
----------------
You can just get rid of the intermediate `S` here and print directly to `OS` in all cases, e.g.:

`OS << "Delete " << T1.showNode(Id1) << "\n"`


https://reviews.llvm.org/D34329