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