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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits