ymandel marked 3 inline comments as done and an inline comment as not done. ymandel added a comment.
Thanks for the review. Added the tests and undid changes to SourceCode. ================ Comment at: clang/include/clang/Tooling/Refactoring/RangeSelector.h:29 + +namespace range_selector { +inline RangeSelector charRange(CharSourceRange R) { ---------------- ilya-biryukov wrote: > Maybe try putting it into the tooling namespace directly? > Or are there immediate and unfortunate conflicts with existing names? > > No conflicts. Was just being cautious. ================ Comment at: clang/include/clang/Tooling/Refactoring/RangeSelector.h:36 +/// \returns the range corresponding to the identified node. +RangeSelector node(StringRef Id); +/// Variant of \c node() that identifies the node as a statement, for purposes ---------------- ilya-biryukov wrote: > NIT: I believe LLVM naming rules would mandate to name it `ID` here and throughout. ================ Comment at: clang/include/clang/Tooling/Refactoring/RangeSelector.h:53 +/// token of the relevant name, not including qualifiers. +RangeSelector name(StringRef Id); + ---------------- ilya-biryukov wrote: > NIT: this is super-specialized, but has a very generic name, therefore might > cause confusion. Maybe call it `ctorInitializerName`? It works with others as well, particularly NamedDecl. This is one place where typed node ids would be nice, b/c that would allow us to make this interface typed, like the matchers. I kept the name but tried to clarify the comments. WDYT? ================ Comment at: clang/include/clang/Tooling/Refactoring/RangeSelector.h:57 +// arguments (all source between the call's parentheses). +RangeSelector args(StringRef Id); + ---------------- ilya-biryukov wrote: > Same thing, maybe rename to `callExprArgs`? > And other nodes too i'd like to keep these as lightweight as possible. Compromised on callArgs? ================ Comment at: clang/include/clang/Tooling/Refactoring/RangeSelector.h:74 +/// `SourceManager::getExpansionRange`. +RangeSelector contraction(RangeSelector S); +} // namespace range_selector ---------------- ilya-biryukov wrote: > NIT: maybe call it `expansion`? > > `contraction` is a new term, might be confusing to use. > > Sigh. You're right. Its just that the existing terminology is confusing. that said, anyone using this combinator is already doing advanced work, so consistency is probably more important than clarity. ================ Comment at: clang/include/clang/Tooling/Refactoring/SourceCode.h:76 + +SourceLocation findPreviousTokenStart(SourceLocation Start, + const SourceManager &SM, ---------------- ilya-biryukov wrote: > Not sure this should be a public API. Maybe keep it private to the use-site? Good point. Originally, we needed these in two different files -- but the range selectors actually obviate that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61774/new/ https://reviews.llvm.org/D61774 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits