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
test/Tooling/clan
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, yo
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
lib/Tooli
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 porti
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 GumTree
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 u
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 s
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 a
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
tools/cl
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 `Node
johannes added inline comments.
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:157
+ int Leaves = 0;
+ std::function Traverse = [&](NodeId Id) {
+const Node &N = getNode(Id);
arphaman wrote:
> you should be able to use `auto` instead of `std::function` here I
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
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 &T1, &T2;
Generally, can we put the public interface first in
arphaman added inline comments.
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:156
+int TreeRoot::getSubtreePostorder(std::vector &Ids, NodeId Root) const
{
+ int Leaves = 0;
+ std::function Traverse = [&](NodeId Id) {
Please use a more descriptive name e.g. `Num
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 wit
johannes added inline comments.
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:171
+
+std::string TreeRoot::label(NodeId Id) const {
+ const Node &N = getNode(Id);
arphaman wrote:
> I believe that this method that you call `label` actually represents the
> `value`
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`?
> N
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
arphaman added inline comments.
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:171
+
+std::string TreeRoot::label(NodeId Id) const {
+ const Node &N = getNode(Id);
I believe that this method that you call `label` actually represents the
`value` attribute that's de
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
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:
> arphaman
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 ImplicitCa
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
test/Tooling/clang-diff-
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.
===
johannes marked 10 inline comments as done.
johannes added inline comments.
Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:123
+
+void runDiff(ASTContext &AST1, ASTContext &AST2);
+
klimek wrote:
> This is the main exposed interface?
>
> Generally, if all w
klimek added inline comments.
Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:123
+
+void runDiff(ASTContext &AST1, ASTContext &AST2);
+
This is the main exposed interface?
Generally, if all we want to do is printing, I wouldn't put that into a library
in T
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
lib/Tooling/ASTDiff/CMakeLists.txt
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
tools/clan
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 lo
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
lib/Tooling
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 cove
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.
https://reviews.l
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 differen
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
tools/clan
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
tools/clang-diff/
35 matches
Mail list logo
