[PATCH] D34329: [GSoC] Clang AST diffing

johannes updated this revision to Diff 104124. https://reviews.llvm.org/D34329 Files: include/clang/Tooling/ASTDiff/ASTDiff.h include/clang/Tooling/ASTDiff/ASTDiffInternal.h lib/Tooling/ASTDiff/ASTDiff.cpp lib/Tooling/ASTDiff/CMakeLists.txt lib/Tooling/CMakeLists.txt

[PATCH] D34329: [GSoC] Clang AST diffing

ruiu added a comment. I'd copy what Hal mentioned in other review thread for other GSoC project. You don't want to tag your patches with "[GSoC]" because it doesn't describe anything about patch contents and many other unrelated patches could have been tagged as single "[GSoC]" tag. Instead,

[PATCH] D34329: [GSoC] Clang AST diffing

johannes updated this revision to Diff 104032. johannes added a comment. refactor https://reviews.llvm.org/D34329 Files: include/clang/Tooling/ASTDiff/ASTDiff.h include/clang/Tooling/ASTDiff/ASTDiffInternal.h lib/Tooling/ASTDiff/ASTDiff.cpp lib/Tooling/ASTDiff/CMakeLists.txt

[PATCH] D34329: [GSoC] Clang AST diffing

johannes added inline comments. Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:730 + +Mapping TreeComparator::matchTopDown() const { + PriorityList L1(T1); johannes wrote: > arphaman wrote: > > Johannes, it seems to me that your implementation of the top-down

[PATCH] D34329: [GSoC] Clang AST diffing

johannes added inline comments. Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:730 + +Mapping TreeComparator::matchTopDown() const { + PriorityList L1(T1); arphaman wrote: > Johannes, it seems to me that your implementation of the top-down portion of > the

[PATCH] D34329: [GSoC] Clang AST diffing

arphaman added inline comments. Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:730 + +Mapping TreeComparator::matchTopDown() const { + PriorityList L1(T1); Johannes, it seems to me that your implementation of the top-down portion of the GumTree algorithm doesn't

[PATCH] D34329: [GSoC] Clang AST diffing

klimek added inline comments. Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:57 +/// Within a tree, this identifies a node by its preorder offset. +using NodeId = int; + johannes wrote: > arphaman wrote: > > I think that it's better to make make `NodeId` a

[PATCH] D34329: [GSoC] Clang AST diffing

johannes added inline comments. Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:57 +/// Within a tree, this identifies a node by its preorder offset. +using NodeId = int; + arphaman wrote: > I think that it's better to make make `NodeId` a structure as well

[PATCH] D34329: [GSoC] Clang AST diffing

johannes updated this revision to Diff 103432. https://reviews.llvm.org/D34329 Files: include/clang/Tooling/ASTDiff/ASTDiff.h lib/Tooling/ASTDiff/ASTDiff.cpp lib/Tooling/ASTDiff/CMakeLists.txt lib/Tooling/CMakeLists.txt test/Tooling/clang-diff-basic.cpp tools/CMakeLists.txt

[PATCH] D34329: [GSoC] Clang AST diffing

arphaman added a comment. The API looks better IMHO. Some more comments: Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:57 +/// Within a tree, this identifies a node by its preorder offset. +using NodeId = int; + I think that it's better to make make

[PATCH] D34329: [GSoC] Clang AST diffing

johannes added inline comments. Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:157 + int Leaves = 0; + std::function Traverse = [&](NodeId Id) { +const Node = getNode(Id); arphaman wrote: > you should be able to use `auto` instead of

[PATCH] D34329: [GSoC] Clang AST diffing

johannes updated this revision to Diff 103409. johannes marked 4 inline comments as done. johannes added a comment. - move some unnecessary things out of the public header Is this a proper way to declutter the header file? Using inheritance would also be possible. I have to define a destructor

[PATCH] D34329: [GSoC] Clang AST diffing

klimek added a comment. Reviewing this mainly from the API view, and leaving the technical details to others :) Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:124 + +class ASTDiff { + TreeRoot , Generally, can we put the public interface first in the

[PATCH] D34329: [GSoC] Clang AST diffing

arphaman added inline comments. Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:156 +int TreeRoot::getSubtreePostorder(std::vector , NodeId Root) const { + int Leaves = 0; + std::function Traverse = [&](NodeId Id) { Please use a more descriptive

[PATCH] D34329: [GSoC] Clang AST diffing

johannes updated this revision to Diff 103320. johannes added a comment. - Fix a bug in getSimilarity() - Change terminology: `label` -> `value` - Define SNodeId: Now it cannot be implicitly constructed from an int, however it can be converted to int. Still feels a bit weird - Fix some issues

[PATCH] D34329: [GSoC] Clang AST diffing

johannes added inline comments. Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:171 + +std::string TreeRoot::label(NodeId Id) const { + const Node = getNode(Id); arphaman wrote: > I believe that this method that you call `label` actually represents the > `value`

[PATCH] D34329: [GSoC] Clang AST diffing

arphaman added inline comments. Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:303 +/// Identifies a node in this subtree by its postorder offset. +using SNodeId = int; + johannes wrote: > arphaman wrote: > > What's the difference between `SNodeId` and `NodeId`? >

[PATCH] D34329: [GSoC] Clang AST diffing

johannes added inline comments. Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:303 +/// Identifies a node in this subtree by its postorder offset. +using SNodeId = int; + arphaman wrote: > What's the difference between `SNodeId` and `NodeId`? NodeId is the preorder

[PATCH] D34329: [GSoC] Clang AST diffing

arphaman added inline comments. Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:171 + +std::string TreeRoot::label(NodeId Id) const { + const Node = getNode(Id); I believe that this method that you call `label` actually represents the `value` attribute that's

[PATCH] D34329: [GSoC] Clang AST diffing

arphaman added inline comments. Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:303 +/// Identifies a node in this subtree by its postorder offset. +using SNodeId = int; + What's the difference between `SNodeId` and `NodeId`? https://reviews.llvm.org/D34329

[PATCH] D34329: [GSoC] Clang AST diffing

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: >

[PATCH] D34329: [GSoC] Clang AST diffing

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

[PATCH] D34329: [GSoC] Clang AST diffing

johannes updated this revision to Diff 103208. johannes marked 7 inline comments as done. https://reviews.llvm.org/D34329 Files: include/clang/Tooling/ASTDiff/ASTDiff.h lib/Tooling/ASTDiff/ASTDiff.cpp lib/Tooling/ASTDiff/CMakeLists.txt lib/Tooling/CMakeLists.txt

[PATCH] D34329: [GSoC] Clang AST diffing

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.

[PATCH] D34329: [GSoC] Clang AST diffing

johannes marked 10 inline comments as done. johannes added inline comments. Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:123 + +void runDiff(ASTContext , ASTContext ); + klimek wrote: > This is the main exposed interface? > > Generally, if all we want to

[PATCH] D34329: [GSoC] Clang AST diffing

klimek added inline comments. Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:123 + +void runDiff(ASTContext , ASTContext ); + 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,

[PATCH] D34329: [GSoC] Clang AST diffing

johannes updated this revision to Diff 103163. johannes added a comment. - Add the option to not use compilation databases. - Add a basic test. https://reviews.llvm.org/D34329 Files: include/clang/Tooling/ASTDiff/ASTDiff.h lib/Tooling/ASTDiff/ASTDiff.cpp

[PATCH] D34329: [GSoC] Clang AST diffing

johannes updated this revision to Diff 103151. https://reviews.llvm.org/D34329 Files: include/clang/Tooling/ASTDiff/ASTDiff.h lib/Tooling/ASTDiff/ASTDiff.cpp lib/Tooling/ASTDiff/CMakeLists.txt lib/Tooling/CMakeLists.txt tools/CMakeLists.txt tools/clang-diff/CMakeLists.txt

[PATCH] D34329: [GSoC] Clang AST diffing

johannes added a comment. In https://reviews.llvm.org/D34329#784090, @arphaman wrote: > Generally we shouldn't have untested code in trunk, so I think that we need > to find a way to test this before committing. We can start off by testing the > output of the diff tool. Since there will be a

[PATCH] D34329: [GSoC] Clang AST diffing

johannes updated this revision to Diff 103147. johannes added a comment. - style fixes - do not compare nodes from system headers https://reviews.llvm.org/D34329 Files: include/clang/Tooling/ASTDiff/ASTDiff.h lib/Tooling/ASTDiff/ASTDiff.cpp lib/Tooling/ASTDiff/CMakeLists.txt

[PATCH] D34329: [GSoC] Clang AST diffing

arphaman added a comment. Generally we shouldn't have untested code in trunk, so I think that we need to find a way to test this before committing. We can start off by testing the output of the diff tool. Since there will be a lot of changes in the future, you don't have to have everything

[PATCH] D34329: [GSoC] Clang AST diffing

arphaman added inline comments. Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:277 +Mappings::~Mappings() { + delete[] SrcToDst; + delete[] DstToSrc; Please use `std::unique_ptr` for `SrcToDst` and `DstToSrc` as well and remove the destructor.

[PATCH] D34329: [GSoC] Clang AST diffing

arphaman added a comment. Congratulations of the first GSoC patch! I have some below comments: - Patches should be submitted using the full context (`git diff -U`). This makes it easier for reviewers to understand the change. This patch mainly adds new code, so this won't make much

[PATCH] D34329: [GSoC] Clang AST diffing

johannes updated this revision to Diff 102986. https://reviews.llvm.org/D34329 Files: include/clang/Tooling/ASTDiff/ASTDiff.h lib/Tooling/ASTDiff/ASTDiff.cpp lib/Tooling/ASTDiff/CMakeLists.txt lib/Tooling/CMakeLists.txt tools/CMakeLists.txt tools/clang-diff/CMakeLists.txt

[PATCH] D34329: [GSoC] Clang AST diffing

johannes created this revision. Herald added subscribers: mgorny, klimek. https://reviews.llvm.org/D34329 Files: include/clang/Tooling/ASTDiff/ASTDiff.h lib/Tooling/ASTDiff/ASTDiff.cpp lib/Tooling/ASTDiff/CMakeLists.txt lib/Tooling/CMakeLists.txt tools/CMakeLists.txt