gribozavr2 accepted this revision.
gribozavr2 added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang/lib/Tooling/Transformer/Parsing.cpp:169
+
+// For consistency with matcher parser and C++ code, node id's are written as
+// strings. However, we do not accept support escaping in the string.
----------------
s/matcher/AST matcher/
================
Comment at: clang/lib/Tooling/Transformer/Parsing.cpp:170
+// For consistency with matcher parser and C++ code, node id's are written as
+// strings. However, we do not accept support escaping in the string.
+static ExpectedProgress<std::string> parseStringId(ParseState State) {
----------------
s/accept//
================
Comment at: clang/lib/Tooling/Transformer/Parsing.cpp:29
+
+namespace {
+using llvm::Error;
----------------
ymandel wrote:
> 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.
> We'd like to move ahead with this patch, but have made the following changes
SGTM.
> the stencil format-string parser (forthcoming) will not easily be merged into
> a lexer-based parser
I'd have to take a closer look to provide a meaningful response, but postponing
this discussion to the patch that will be adding this code SGTM.
================
Comment at: clang/unittests/Tooling/RangeSelectorTest.cpp:271
+ ASSERT_THAT_EXPECTED(R, llvm::Succeeded());
+ EXPECT_THAT_EXPECTED(select(*R, Match), HasValue("3;"));
}
----------------
ymandel wrote:
> 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?
> I propose that we move RangeSelector to implement MatchComputation rather
> than MatchConsumer to support toString and better tests. WDYT?
+1, makes sense. Initially they were separate abstractions because we thought
we didn't need the complexity everywhere, but turns out we do.
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
