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