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

[Manuel Klimek via Phabricator via cfe-commits]

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 it.


================
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);
----------------
Any reason not to do multiple ranges?

Also, it's not clear from reading the description here why we need the Location.


================
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:100
+      SelectionStack.back().Children.push_back(std::move(Node));
+    return true;
+  }
----------------
Why do we always stop traversal?


================
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:103
+
+  void TraverseDeclInPrevious(Decl *D) {
+    assert(!SelectionStack.back().Children.empty() &&
----------------
A short comment explaining when this happens would help understand this.


================
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:164
+  unsigned NumMatches = 0;
+  for (Decl *D : Context.getTranslationUnitDecl()->decls()) {
+    if (ObjCImplEndLoc.isValid() &&
----------------
Why don't we do this as part of TraverseDecl() in the visitor?


================
Comment at: lib/Tooling/Refactoring/SourceLocationUtils.h:1
+//===--- SourceLocationUtils.h - Source location helper functions ---------===//
+//
----------------
I'm somewhat opposed to files having "utils" in the name, as they tend to accumulate too much stuff.
Can we implement those in SourceLocation and SourceManager respectively?


Repository:
  rL LLVM

https://reviews.llvm.org/D35012