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