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