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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 7 inline comments as done.
ymandel added inline comments.


================
Comment at: clang/include/clang/Tooling/Refactoring/RangeSelector.h:29
+
+namespace range_selector {
+inline RangeSelector charRange(CharSourceRange R) {
----------------
ilya-biryukov wrote:
> 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.
SGTM. 


================
Comment at: clang/unittests/Tooling/RangeSelectorTest.cpp:143
+  // Node-id specific version:
+  test(Matcher, range(Arg0, Arg1), Code, "3, 7");
+  // General version:
----------------
ilya-biryukov wrote:
> 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());
> ```
Thanks for the suggestion. Thoroughly reworked the tests along these lines. I think the result is much clearer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61774