[PATCH] D34329: [GSoC] Clang AST diffing

[Johannes Altmanninger via Phabricator via cfe-commits]

johannes marked an inline comment as done.
johannes added a comment.

In https://reviews.llvm.org/D34329#785190, @arphaman wrote:

> Looking at the output of the tool, I have the following suggestion:
>
> - We should avoid implicit expressions (We don't need to see things like `Insert ImplicitCastExpr(21) into BinaryOperator: *(22) at 0`). This can be done in a follow-up patch though.


Ok, I will include that and other features in future patches.



================
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;
----------------
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?


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:693
+      H2 = L2.pop();
+      for (NodeId Id1 : H1)
+        for (NodeId Id2 : H2)
----------------
arphaman wrote:
> Please wrap these loops in braces.
Not sure if I got this right.


================
Comment at: test/Tooling/clang-diff-basic.cpp:3
+// RUN: awk '/^.. dst/{f=1;next}/^.. end dst/{f=0}f' %s > %T/dst.cpp
+// RUN: clang-diff -no-compilation-database %T/src.cpp %T/dst.cpp
+// RUN: clang-diff -no-compilation-database %T/src.cpp %T/dst.cpp | FileCheck %s
----------------
arphaman wrote:
> I'd prefer it if we used something like `clang -E` and preprocessor to generate the two files.
> 
> E.g.:
> 
> ```
> RUN: %clang_cc1 -E %s > %T/src.cpp
> RUN: %clang_cc1 -E %s -D DEST > %T/dst.cpp
> #ifndef DEST
> namespace src { };
> #else
> namespace dst { };
> #endif
> ```
Yes, much better!


================
Comment at: test/Tooling/clang-diff-basic.cpp:4
+// RUN: clang-diff -no-compilation-database %T/src.cpp %T/dst.cpp
+// RUN: clang-diff -no-compilation-database %T/src.cpp %T/dst.cpp | FileCheck %s
+
----------------
arphaman wrote:
> Why do you need two invocations of `clang-diff` with the same arguments?
That was unintentional, I removed it.


https://reviews.llvm.org/D34329