[PATCH] D81868: [libTooling] Add parser for string representation of `RangeSelector`.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.


================
Comment at: clang/include/clang/Tooling/Transformer/Parsing.h:1
+//===--- Parsing.h - Parsing library for Transformer ---------*- C++ -*-===//
+//
----------------
Please pad the first line to match the other line (throughout the patch).


================
Comment at: clang/include/clang/Tooling/Transformer/Parsing.h:27
+
+/// Parses a string-representation of a \c RangeSelector. The grammar of these
+/// strings is closely based on the (sub)grammar of \c RangeSelectors as they'd
----------------
"string representation"


================
Comment at: clang/lib/Tooling/Transformer/Parsing.cpp:29
+
+namespace {
+using llvm::Error;
----------------
I'm a bit concerned about the abundance of parsers in Clang tooling: we already have a similar language for dynamic AST matchers. I'm concerned about both code duplication as such, and that there are multiple ways to do something.

Code duplication makes it more difficult to carry out horizontal efforts like improving error reporting.

Multiple ways to do something makes codebase knowledge less reusable. It might also create language discrepancies that users might notice (for example, I don't remember if `bind(id)` works in dynamic AST matchers or not; we would be better off if range selectors were consistent with that).

I don't think this particular change decisively tips the scale towards refactoring the parser for dynamic AST matchers to be reusable here; however it is an option worth considering. I think we should be thinking about the total cost of ownership of this code.

Some future use cases will also need an embeddable language (like AST matchers for the syntax tree, or parsing stencils from strings).


================
Comment at: clang/unittests/Tooling/RangeSelectorTest.cpp:271
+  ASSERT_THAT_EXPECTED(R, llvm::Succeeded());
+  EXPECT_THAT_EXPECTED(select(*R, Match), HasValue("3;"));
 }
----------------
I wonder if we could figure out a more direct testing strategy for a parser (that does not necessarily involve using the parsed objects) if we had more advanced parsing infrastructure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81868