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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks! Mostly NITs from me, the design looks nice! Would you mind adding some tests before we land this?

The only major thing that I'd argue against is adding helper functions like `findPreviousToken` to `SourceCode.h`.
It's fine to have them in the `.cpp` files if they make the implementation simpler, but we want to design them more carefully if we put them in the public API.



================
Comment at: clang/include/clang/Tooling/Refactoring/RangeSelector.h:29
+
+namespace range_selector {
+inline RangeSelector charRange(CharSourceRange R) {
----------------
Maybe try putting it into the tooling namespace directly?
Or are there immediate and unfortunate conflicts with existing names?




================
Comment at: clang/include/clang/Tooling/Refactoring/RangeSelector.h:35
+
+/// \returns the range corresponding to the identified node.
+RangeSelector node(StringRef Id);
----------------
NIT: maybe explain what Id is?


================
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
----------------
NIT: I believe LLVM naming rules would mandate to name it  `ID`


================
Comment at: clang/include/clang/Tooling/Refactoring/RangeSelector.h:37
+RangeSelector node(StringRef Id);
+/// Variant of \c node() that identifies the node as a statement, for purposes
+/// of deciding whether to include any trailing semicolon in the selected range.
----------------
Maybe a shorter description could suffice?
```
Selects a node and a trailing semicolon. Useful for selecting expression statements.
```


================
Comment at: clang/include/clang/Tooling/Refactoring/RangeSelector.h:43
+/// statement.
+RangeSelector sNode(StringRef Id);
+
----------------
NIT: maybe name it `statement`?


================
Comment at: clang/include/clang/Tooling/Refactoring/RangeSelector.h:46
+/// Convenience version of \c range where end points are nodes.
+RangeSelector nodeRange(StringRef BeginId, StringRef EndId);
+
----------------
Could this be an overload with the same name?


================
Comment at: clang/include/clang/Tooling/Refactoring/RangeSelector.h:53
+/// token of the relevant name, not including qualifiers.
+RangeSelector name(StringRef Id);
+
----------------
NIT: this is super-specialized, but has a very generic name, therefore might cause confusion. Maybe call it `ctorInitializerName`?


================
Comment at: clang/include/clang/Tooling/Refactoring/RangeSelector.h:57
+// arguments (all source between the call's parentheses).
+RangeSelector args(StringRef Id);
+
----------------
Same thing, maybe rename to `callExprArgs`?
And other nodes too


================
Comment at: clang/include/clang/Tooling/Refactoring/RangeSelector.h:74
+/// `SourceManager::getExpansionRange`.
+RangeSelector contraction(RangeSelector S);
+} // namespace range_selector
----------------
NIT: maybe call it `expansion`? 

`contraction` is a new term, might be confusing to use.




================
Comment at: clang/include/clang/Tooling/Refactoring/SourceCode.h:76
+
+SourceLocation findPreviousTokenStart(SourceLocation Start,
+                                      const SourceManager &SM,
----------------
Not sure this should be a public API. Maybe keep it private to the use-site?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61774/new/

https://reviews.llvm.org/D61774