[PATCH] D61774: [LibTooling] Add RangeSelector library for defining source ranges based on bound AST nodes.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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