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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.


================
Comment at: clang/include/clang/Tooling/Refactoring/RangeSelector.h:29
+
+namespace range_selector {
+inline RangeSelector charRange(CharSourceRange R) {
----------------
ymandel wrote:
> 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.
I can see why a separate namespace would make sense, but since the `tooling` namespace  is not densely populated I hope we can get away with putting things here directly and saving some typing on the use-sites.

Hope that does not backfire in the future, though.


================
Comment at: clang/include/clang/Tooling/Refactoring/RangeSelector.h:53
+/// token of the relevant name, not including qualifiers.
+RangeSelector name(StringRef Id);
+
----------------
ymandel wrote:
> 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?
Ah, sorry, misread the original comment. The name actually makes sense in that case.
Am I correct to assume it will only select the final identifier of a qualified name, but not the qualifier?
E.g. for `::foo::bar::baz`, it would only select `baz`, right?
What happens when we also have template args? E.g. for `::foo::bar::baz<int>`, it would only select `baz`, right?

Maybe add those examples to the documentation? It's something that's very easy to get wrong.


================
Comment at: clang/include/clang/Tooling/Refactoring/RangeSelector.h:57
+// arguments (all source between the call's parentheses).
+RangeSelector args(StringRef Id);
+
----------------
ymandel wrote:
> 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?
`callArgs` LG, thanks!


================
Comment at: clang/unittests/Tooling/RangeSelectorTest.cpp:39
+llvm::Optional<TestMatch> matchAny(StringRef Code, M Matcher) {
+  auto AstUnit = buildASTFromCode(Code);
+  if (AstUnit == nullptr) {
----------------
NIT: use `ASTUnit` to match LLVM naming rules


================
Comment at: clang/unittests/Tooling/RangeSelectorTest.cpp:143
+  // Node-id specific version:
+  test(Matcher, range(Arg0, Arg1), Code, "3, 7");
+  // General version:
----------------
Consider restructuring the code to place assertions into the test function itself.
This wildly improves locations reported in case of test failures and makes tests easier to read.

I.e. having something like
```
auto AST = buildASTAndMatch(Code, Matcher);
EXPECT_EQ(applySelector(range(Arg0, Arg1), AST),  "3, 7");
```
is arguably both easier to read and produces better location information on failures than a function that runs the test.
Even though it's a bit more code.


Note that it's a bit more complicated if you need to deal with `Expected<>` return types, but still possible:
```
EXPECT_THAT_EXPECTED(applySelector(range(Arg0, Arg1), AST),  HasValue("3, 7"));
EXPECT_THAT_EXPECTED(applySelector(range(Arg1, Arg0), AST),  Failed());
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61774