[PATCH] D35012: [refactor] Add the AST source selection component

[Alex Lorenz via Phabricator via cfe-commits]

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