ymandel marked 6 inline comments as done.
ymandel added a comment.
Thanks for the review!
================
Comment at: clang/lib/Tooling/Transformer/Parsing.cpp:29
+
+namespace {
+using llvm::Error;
----------------
gribozavr2 wrote:
> 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).
Agreed on these points. We'd like to move ahead with this patch, but have made
the following changes:
1. Added a FIXME in the implementation file indicating intent to merge with AST
matcher parser code
2. Modified the accepted language for consistency with AST matchers (and C++
code). Node ids must be surrounded by quotes.
One possible complication is that the stencil format-string parser
(forthcoming) will not easily be merged into a lexer-based parser, given that
it allows free text for the format string (only the special escape sequences
have specified structure). So, we will need to find a way to operate both
scannerless and not if we want a unified parser infrastructure. However, we can
solve that problem when we come to it.
================
Comment at: clang/unittests/Tooling/RangeSelectorTest.cpp:271
+ ASSERT_THAT_EXPECTED(R, llvm::Succeeded());
+ EXPECT_THAT_EXPECTED(select(*R, Match), HasValue("3;"));
}
----------------
gribozavr2 wrote:
> 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.
Indeed. That would certainly be preferable. `Stencil`s support the `toString`
operator for just such testing support. A generic parse tree would also allow
direct testing, but then you still need to find a way to test the conversion
from parse-tree to stencil. So, ultimately, you want to have some reflection on
the type itself.
I propose that we move `RangeSelector` to implement `MatchComputation` rather
than `MatchConsumer` to support `toString` and better tests. WDYT?
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
