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 corresponds to the TranslationUnitDecl otherwise.
+Optional<SelectedASTNode> findSelectedASTNodes(const ASTContext &Context,
+ SourceLocation Location,
+ SourceRange SelectionRange);
----------------
arphaman wrote:
> klimek wrote:
> > Any reason not to do multiple ranges?
> >
> > Also, it's not clear from reading the description here why we need the
> > Location.
> I guess multiple ranges can be supported in follow-up patches to keep this
> one simpler.
>
> I'll add it to the comment, but the idea is that the location corresponds to
> the cursor/right click position in the editor. That means that location
> doesn't have to be identical to the selection, since you can select something
> and then right click anywhere in the editor.
I've decided to get rid of `Location` and `ContainsSelectionPoint` at this
level. I will instead handle this editor specific thing at a higher-level and
make selection search range only to simplify things at this level.
================
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:100
+ SelectionStack.back().Children.push_back(std::move(Node));
+ return true;
+ }
----------------
klimek wrote:
> arphaman wrote:
> > klimek wrote:
> > > Why do we always stop traversal?
> > False stops traversal, true is the default return value.
> Ah, right. Generally, I'd have expected us to stop traversal once we're past
> the range?
We already do stop traversal early in `findSelectedASTNodes` once we match
everything that's possible (although if the selection doesn't overlap with
anything we will traverse all top level nodes). I see the point though, we
might as stop after the range as well. I added the check to do that in the
updated patch.
================
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:164
+ unsigned NumMatches = 0;
+ for (Decl *D : Context.getTranslationUnitDecl()->decls()) {
+ if (ObjCImplEndLoc.isValid() &&
----------------
klimek wrote:
> Why don't we do this as part of TraverseDecl() in the visitor?
I think it's easier to handle the Objective-C `@implementation` logic here,
unless there's some better way that I can't see ATM.
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
