sylvestre.ledru added a comment.
@johannes Someone asked for that in Debian:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=907269
Repository:
rL LLVM
https://reviews.llvm.org/D34329
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
johannes added a comment.
In https://reviews.llvm.org/D34329#1213667, @sylvestre.ledru wrote:
> @arphaman @johannes Is that normal that clang-diff isn't installed by cmake?
> (like clang-format?)
Yes, we did not add that. I don't know if anyone would use it.
Repository:
rL LLVM
sylvestre.ledru added a comment.
Herald added subscribers: llvm-commits, kadircet, mgrang.
@arphaman @johannes Is that normal that clang-diff isn't installed by cmake?
(like clang-format?)
Repository:
rL LLVM
https://reviews.llvm.org/D34329
___
This revision was automatically updated to reflect the committed changes.
Closed by commit rL308731: [clang-diff] Add initial implementation (authored by
arphaman).
Changed prior to commit:
https://reviews.llvm.org/D34329?vs=105161=107661#toc
Repository:
rL LLVM
arphaman added a comment.
I'll commit this on behalf of Johannes today as he didn't get his access yet
https://reviews.llvm.org/D34329
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
teemperor added a comment.
@johannes https://reviews.llvm.org/D34880 has landed, so feel free to propose
patches to the StmtDataCollector API that would help you (e.g. to support
identifiers). You can see examples how to use it in the CloneDetection.cpp
(once for storing data in a
arphaman accepted this revision.
arphaman added a comment.
This revision is now accepted and ready to land.
LGTM. You can request commit access at
http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access.
https://reviews.llvm.org/D34329
johannes updated this revision to Diff 105161.
johannes added a comment.
- style fixes
- correct getSimilarity()
https://reviews.llvm.org/D34329
Files:
include/clang/Tooling/ASTDiff/ASTDiff.h
include/clang/Tooling/ASTDiff/ASTDiffInternal.h
lib/Tooling/ASTDiff/ASTDiff.cpp
arphaman added inline comments.
Comment at: include/clang/Tooling/ASTDiff/ASTDiffInternal.h:38
+ operator int() const { return Id; }
+ NodeId ++() { return ++this->Id, *this; }
+ NodeId () { return --this->Id, *this; }
NIT: You don't need `this` here or in
arphaman added inline comments.
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:359
+
+ explicit SNodeId(int Id) : Id(Id){};
+ explicit SNodeId() = default;
NIT: This ';' is redundant.
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:363
+ operator
arphaman added inline comments.
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:385
+ }
+ int getSizeS() const { return RootIds.size(); }
+ NodeId getIdInRoot(SNodeId Id) const {
What's the purpose of the `S` prefix in the name of this method and a couple of
johannes added a comment.
In https://reviews.llvm.org/D34329#798377, @arphaman wrote:
> @johannes
> Are you planning to work on integration with the `StmtDataCollector` in this
> patch or would you prefer to follow-up with additional patches?
Later would be better
arphaman added a comment.
@johannes
Are you planning to work on integration with the `StmtDataCollector` in this
patch or would you prefer to follow-up with additional patches?
https://reviews.llvm.org/D34329
___
cfe-commits mailing list
johannes updated this revision to Diff 105059.
johannes edited the summary of this revision.
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
teemperor added a comment.
Yes, it does indeed skip identifiers and literals for this reason :). It was
planned to make this template more configurable for use cases like yours, so
I'm totally fine with adding configuration parameters. I just opened
https://reviews.llvm.org/D34880 where I make
johannes marked an inline comment as done.
johannes added a comment.
In https://reviews.llvm.org/D34329#796574, @teemperor wrote:
> I didn't have time to have a close look at this patch, but it seems you're
> interested in the specific TU-independent data of a Stmt to compare them. So
> if you
teemperor added a comment.
I didn't have time to have a close look at this patch, but it seems you're
interested in the specific TU-independent data of a Stmt to compare them. So if
you are interested in the such data and don't want to write your own function
to collect data it for each Stmt
arphaman added a comment.
I think that it's pretty much ready. I think that the test should be expanded
though. At the very least it should check that all of the node types that are
supported by `SyntaxTreeImpl::getNodeValueImpl` get matched.
https://reviews.llvm.org/D34329
johannes marked 2 inline comments as done.
johannes added inline comments.
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:439
+// Computes an optimal mapping between two trees.
+class ZsMatcher {
+ const ASTDiff::Impl
arphaman wrote:
> Do you know the reason for
johannes updated this revision to Diff 104664.
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
arphaman added inline comments.
Comment at: include/clang/Tooling/ASTDiff/ASTDiffInternal.h:184
+
+ std::string getNodeValueI(NodeId Id) const;
+ std::string getNodeValueI(const DynTypedNode ) const;
`getNodeValueImpl`?
Comment at:
johannes updated this revision to Diff 104401.
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
johannes updated this revision to Diff 104395.
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
johannes updated this revision to Diff 104394.
johannes added a comment.
- remove unused struct
- rename getNodeValueI -> getNodeValueImpl
https://reviews.llvm.org/D34329
Files:
include/clang/Tooling/ASTDiff/ASTDiff.h
include/clang/Tooling/ASTDiff/ASTDiffInternal.h
johannes updated this revision to Diff 104249.
johannes added a comment.
- pass Options as a const reference instead of a pointer
- rename TreeComparator -> ASTDiff::Impl, rename Comparator -> DiffImpl
- move declaration of ASTDiff::Impl from the header to the source file so that
Options does
arphaman added inline comments.
Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:57
+ SyntaxTree(T *Node, const ASTContext )
+ : TreeImpl(llvm::make_unique(this, Node, AST)) {}
+
If you want to use two different names then something like
arphaman added a comment.
Ignore the "Just to clarify: I'm fine with adding the" comment, it was from
last week that was saved in my session and that I didn't delete.
https://reviews.llvm.org/D34329
___
cfe-commits mailing list
arphaman added a comment.
Just to clarify: I'm fine with adding the
Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:32
+public:
+ ASTDiff(SyntaxTree , SyntaxTree , ComparisonOptions *Options);
+
Can you pass-in the options by value instead of a pointer?
28 matches
Mail list logo
