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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
