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