Re: Reminder: Review for Zach's parsing library starts on Feb 19th

From: Mohammad Nejati [ashtum] (ashtumashtum_at_[hidden])
Date: 2024-03-01 17:37:34

Hi everyone,

This is my review of the proposed Boost.Parser. I'm posting my review
without an Accept/Reject vote.

# Are you knowledgeable about the problem domain?

I've had no prior experience using parser combinators. Therefore, I'm
writing this review as someone who attempted to read the documentation
and utilize the library to create a parser for parsing chunk-encoded
HTML bodies.

# How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

I've spent approximately 1 day reading the documentation and 1 day
experimenting with the code and comparing it with Boost.Spirit.

# Does this library bring real benefit to C++ developers for
real-world use-case?

Yes, especially given that we frequently encounter applications or
libraries that necessitate parsing strings or plain text network
protocols. Hand-crafted parsers are prone to bugs and require thorough
reviews and fuzz tests to ensure accuracy. Having a library that
enables the construction of complex parsers by combining simple and
verifiable rules can significantly reduce development time.

# Do you have an application for this library?

I find it useful for testing serializers and parsers in network
libraries that implement a plain-text network protocol like HTTP. We
can load test the serializer or parts of it against simple parsers
made using this library, in addition to the more complex parser that
is built for efficiency but is more bug-prone.

# What is your evaluation of the documentation?

I noticed a few areas where the documentation could be improved:
1. The documentation assumes that the reader is already familiar with
using former parsers in Boost libraries like Spirit.QI, making it
nearly unusable for those who lack prior knowledge of Spirit.
2. There are very few examples demonstrating how to use directives,
such as 'repeat', for instance.
3. There is a lack of a step-by-step guide that explains fundamental
concepts through examples.
4. The table of contents is either overly verbose or not categorized
properly, making it difficult to locate related sections.

# What is your evaluation of the design?

I found the necessity to go through multiple steps and use macros for
defining rules a bit cumbersome. It would be much more ergonomic to
enable the definition of rules in a single line, if possible.

# Did you try to use the library? With what compiler? Did you have any problems?

Parsing chunk-encoded HTML bodies requires the utilization of
attributes from a preceding parser to extract each chunk. I found the
placeholders in Boost.Spirit very handy for accessing attributes and
local variables. here is how the parser appears in Boost.Spirit:
```
chunk_rule %= qi::hex[_a = _1] >> qi::eps(_a) >> "\r\n" >>
qi::repeat(_a)[qi::char_] >> "\r\n";
```
In comparison to using lambda expressions and external state variables
in the proposed library:
```
unsigned int size = 0;
auto chunk_parser =
bp::hex[([&size](auto & ctx) { size = bp::_attr(ctx); })] >>
bp::eps([&size](auto &) { return size != 0; }) >> "\r\n" >>
bp::repeat(std::ref(size))[bp::char_] >> "\r\n";
```
I attempted to define a rule with local variables as well, but there
was no way to access the variable within the repeat directive. I'm
unsure if it's technically feasible, but it would be very ergonomic to
write concise code like using placeholders in Boost.Spirit.

Another issue was the fact writing:
```
bp::repeat(std::ref(size))[bp::char_]
```
Is not an efficient method for extracting a chunk of string. This
approach loops and executes the `bp::char_ parser` on each character
and there is no parser available that can chunk a portion of the input
string.

In summary, my experience wasn't great. I anticipated a simple parser
that could be written in one line and was easy to reason about.
However, it ended up requiring manual state management and was quite
convoluted.

I'd like to thank Zach for his work, and I hope this review might
prove useful for improving the library.

- Affiliation disclosure

Please note that I'm affiliated with the C++ Alliance.

Regards,
Mohammad Nejati

On Fri, Mar 1, 2024 at 3:43 PM Andrzej Krzemienski via Boost
<boost_at_[hidden]> wrote:
>
> czw., 29 lut 2024 o 23:30 Christian Mazakas via Boost <boost_at_[hidden]>
> napisał(a):
>
> > I'm not entirely sold on the value of Hana's `operator[]` for tuple
> > accesses.
> >
> > Especially because in later C++, we have placeholders so we can always just
> > do:
> >
> >   auto const& [first,_,_] = my_tuple;
> >
> > https://godbolt.org/z/vvnKxjGos
> >
> > I largely agree with Alan, Hana's a heavyweight dependency for what I think
> > is dubious
> > benefit. Maybe others feel differently here but I'd prefer to just always
> > work with the
> > standard tuple and structured bindings than Hana's `operator[]`.
>
>
> Once/if https://github.com/tzlaine/parser/issues/106 is implemented. The
> motivation for using Hana over std::tuple will be reduced practically to
> zero.
> https://github.com/tzlaine/parser/issues/106 handles the simple cases, and
> for more complicated logic, you do not mind calling std::get<>.
>
> Regards,
> &rzej;
>
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On Mon, Feb 12, 2024 at 7:34 PM Marshall Clow via Boost
<boost_at_[hidden]> wrote:
>
> That’s a week from now.
>
> Github: https://github.com/tzlaine/parser
> Docs:   https://tzlaine.github.io/parser
>
>
> — Marshall
>
>
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost