arphaman added a comment.
r311664
Repository:
rL LLVM
https://reviews.llvm.org/D35012
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
krasimir added a comment.
The test `CursorAtStartOfFunction` is segfaulting.
Repository:
rL LLVM
https://reviews.llvm.org/D35012
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL311655: [refactor] Add the AST source selection component
(authored by arphaman).
Changed prior to commit:
https://reviews.llvm.org/D35012?vs=108078=112550#toc
Repository:
rL LLVM
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
Yep. Lgtm!
Repository:
rL LLVM
https://reviews.llvm.org/D35012
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
klimek added a comment.
Apart from nits, looks OK to me - Eric, are all your concerns addressed?
Comment at: include/clang/Tooling/Refactoring/ASTSelection.h:51-53
+ ast_type_traits::DynTypedNode Node;
+ SourceSelectionKind SelectionKind;
+ std::vector Children;
arphaman added a comment.
Ping.
Repository:
rL LLVM
https://reviews.llvm.org/D35012
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
arphaman updated this revision to Diff 108078.
arphaman added a comment.
Simplified the implementation of `LexicallyOrderedRecursiveASTVisitor` and
removed redundant DenseMap checks.
Ping.
Repository:
rL LLVM
https://reviews.llvm.org/D35012
Files:
arphaman updated this revision to Diff 107138.
arphaman added a comment.
rm early exit bug
Repository:
rL LLVM
https://reviews.llvm.org/D35012
Files:
include/clang/AST/LexicallyOrderedRecursiveASTVisitor.h
include/clang/Basic/SourceLocation.h
include/clang/Basic/SourceManager.h
arphaman added a comment.
Oops, I just realised that now there's a small bug with early exits. Since we
don't actually have true lexical order for declarations in the @implementation
we might exit early after visiting a method in the @implementation before a
function that's actually written
arphaman updated this revision to Diff 107121.
arphaman added a comment.
Factor out the lexical ordering code into a new visitor and simplify the
implementation of the ast selection visitor
Repository:
rL LLVM
https://reviews.llvm.org/D35012
Files:
klimek added inline comments.
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:164
+ unsigned NumMatches = 0;
+ for (Decl *D : Context.getTranslationUnitDecl()->decls()) {
+if (ObjCImplEndLoc.isValid() &&
arphaman wrote:
> klimek wrote:
> > arphaman
arphaman added inline comments.
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:164
+ unsigned NumMatches = 0;
+ for (Decl *D : Context.getTranslationUnitDecl()->decls()) {
+if (ObjCImplEndLoc.isValid() &&
klimek wrote:
> arphaman wrote:
> > klimek
klimek added inline comments.
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:164
+ unsigned NumMatches = 0;
+ for (Decl *D : Context.getTranslationUnitDecl()->decls()) {
+if (ObjCImplEndLoc.isValid() &&
arphaman wrote:
> klimek wrote:
> > Why don't
arphaman added inline comments.
Comment at: include/clang/Tooling/Refactoring/ASTSelection.h:68-74
+/// Traverses the given ASTContext and creates a tree of selected AST nodes.
+///
+/// \returns None if no nodes are selected in the AST, or a selected AST node
+/// that
arphaman updated this revision to Diff 107067.
arphaman marked 7 inline comments as done.
arphaman added a comment.
- Address review comments.
- Remove the `Location` parameter and `ContainsSelectionPoint` enum value.
- Stop traversing early when a declaration that ends after the selection range
klimek added inline comments.
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:100
+ SelectionStack.back().Children.push_back(std::move(Node));
+return true;
+ }
arphaman wrote:
> klimek wrote:
> > Why do we always stop traversal?
> False stops
arphaman added inline comments.
Comment at: include/clang/Tooling/Refactoring/ASTSelection.h:68-74
+/// Traverses the given ASTContext and creates a tree of selected AST nodes.
+///
+/// \returns None if no nodes are selected in the AST, or a selected AST node
+/// that
ioeric added a comment.
Some nits.
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:23
+/// of the cursor or overlap with the selection range.
+class ASTSelectionFinder : public RecursiveASTVisitor {
+ const SourceLocation Location;
In LLVM, we use the
klimek added inline comments.
Comment at: include/clang/Tooling/Refactoring/ASTSelection.h:30
+ /// inside of its source range.
+ ContainsSelectionPoint,
+
It's unclear what a selection point is. I'd have expected this to mean
"overlaps" when first reading
arphaman added a comment.
Ping.
Repository:
rL LLVM
https://reviews.llvm.org/D35012
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
arphaman updated this revision to Diff 105269.
arphaman added a comment.
A a test-case for implicit declarations.
Repository:
rL LLVM
https://reviews.llvm.org/D35012
Files:
include/clang/Tooling/Refactoring/ASTSelection.h
lib/Tooling/Refactoring/ASTSelection.cpp
arphaman created this revision.
Herald added a subscriber: mgorny.
This patch adds the base AST source selection component to the refactoring
library. AST selection is represented using a tree of `SelectedASTNode` values.
Each selected node gets its own selection kind, which can actually be
22 matches
Mail list logo
