[PATCH] D34329: [GSoC] Clang AST diffing

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:474
+    for (SNodeId D1 = LMD1 + 1; D1 <= Id1; ++D1) {
+      double DeletionCost = 1.0;
+      ForestDist[D1][LMD2] = ForestDist[D1 - 1][LMD2] + DeletionCost;
----------------
johannes wrote:
> arphaman wrote:
> > Are the `DeletionCost` and `InsertionCost` constants or are you planning to modify them in the future?
> I think they could be modified for minor improvements of the matching, but I am probably not going to do so anytime soon. Maybe it is better to store them at class scope as static const / constexpr?
Yeah, I suppose for now they should be declared as constants (the static const / constexpr doesn't matter). They should probably be documented as well.


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:693
+      H2 = L2.pop();
+      for (NodeId Id1 : H1)
+        for (NodeId Id2 : H2)
----------------
johannes wrote:
> arphaman wrote:
> > Please wrap these loops in braces.
> Not sure if I got this right.
It looks better, thanks. Generally we prefer to use braces for `if`/`for`/etc. if they have more than one statement.


https://reviews.llvm.org/D34329