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 
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 
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());

  rG LLVM Github Monorepo


cfe-commits mailing list

Reply via email to