[PATCH] D34329: [GSoC] Clang AST diffing

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

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.



================
Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:25
+
+using namespace llvm;
+using namespace clang;
----------------
There's no need to include the `using namespace` declarations in the header.


================
Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:35
+/// Sentinel value for invalid nodes.
+const NodeId NoNodeId = -1;
+
----------------
`InvalidNodeId` sounds better IMHO.


================
Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:123
+
+void runDiff(ASTContext &AST1, ASTContext &AST2);
+
----------------
johannes wrote:
> klimek wrote:
> > This is the main exposed interface?
> > 
> > Generally, if all we want to do is printing, I wouldn't put that into a library in Tooling, but just implement a tools/ASTDiffer or somesuch.
> > 
> > If you want to make this a library, it should return the diff in some form that's nice to use (or print).
> > 
> I started out by creating a self contained tool, that's why the interface does not really make sense.
> 
> I will change it to provide the mappings and the edit script in a nice way, this might be quite useful.
I agree with Manuel here. We should move `runDiff` to the tool and and expose the `ClandDiff` interface in the header.


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:170
+
+std::string TreeRoot::getSourceString(SourceLocation SourceBegin,
+                                      SourceLocation SourceEnd) const {
----------------
Please use `Lexer::getSourceText` instead of this custom function.


================
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;
----------------
Are the `DeletionCost` and `InsertionCost` constants or are you planning to modify them in the future?


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:591
+    const Node &N2 = T2.getNode(Id2);
+    for (size_t Id = 0; Id < N1.Children.size(); ++Id)
+      addIsomorphicSubTrees(M, N1.Children[Id], N2.Children[Id]);
----------------
The `for` loops in LLVM typically store the end value in a separate variable, e.g:

`for (size_t Id = 0, E = N1.Children.size(); Id < E; ++Id)`

Please update this and similar `for` loops in this patch.


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:693
+      H2 = L2.pop();
+      for (NodeId Id1 : H1)
+        for (NodeId Id2 : H2)
----------------
Please wrap these loops in braces.


================
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
----------------
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
```


================
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
+
----------------
Why do you need two invocations of `clang-diff` with the same arguments?


https://reviews.llvm.org/D34329