Boost logo

Boost :

From: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2024-03-01 00:36:20

On Thu, Feb 29, 2024 at 7:16 AM Дмитрий Архипов via Boost
<boost_at_[hidden]> wrote:
> This is my review of Boost.Parser.


> # What is your evaluation of the documentation?
> Currently documentation looks a bit undercooked. It needs some restructuring.
> For example, on one page there is a table of attributes for library-provided
> parsers. On another, there is a table of attributes for parser operators. On
> yet another those two pages are repeated, and also there's a bunch of
> extra rules for
> how sequences and alternatives affect attributes, followed by a table with
> examples. This creates some confusion at first on where to look up information
> on attributes.

Hopefully, this is addressed by the Cheat Sheet I made. It has all
the tables copied into one place.

> Another complaint I have is that the fact that the attribute for
> parsers with semantic
> actions is none is not advertised well enough. This is a very important piece of
> information, because most parsers use actions. And yet, this is only mentioned
> in operator tables, and never mentioned in the section on semantic actions.

Right, good point. Ticket:

> Also, I had a lot of success using _locals. But the docs only mention it in
> passing. I feel like overall the documentation would have benefited from more
> examples which solve some specific tasks using various library components.

I agree; there's already a ticket open for this one.


> One thing I want to note is that the library seems to have copied several
> naming conventions from Spirit. I'm not convinced there's much value in naming
> context accessors with a leading underscore. I'm also not convinced dependency
> on Boost.Hana for the sole purpose of using its tuple's operator[] is
> warranted.

I feel strongly that it is, having used op[] a lot. That being said,
I'm becoming aware that this is a minority opinion. Perhaps I should
make std::tuple the default, instead of the other way around. I don't
want to remove hana::tuple entirely, because I want to use it in my
own parsers.


> While I was writing those parsers using trace::on was of great help for
> debugging errors. I still had a bunch of several pages-long template errors,
> particularly when parser attributes were not of the type I expected it to be.
> A related issue is that I have been using the branch that allowed returning
> values from semantic actions, and the library often couldn't correctly figure
> out whether I wanted to pass a context to the action, or just the attribute.
> In the end, I had to add explicit return type of void to all of the former.
> With the latter the problem sometimes manifested when a lambda had several
> return statements. I predict that this will be a significant usability issue.

Yeah, I implemented it to see how it works, and it only kind of does.
I might experiment with alternatives that cannot be ambiguous, or
might strike it. It was an interesting idea, but being forced to
rigorously constrain your lambda's is not fun.

> Several people have asked for a way to define rules without a macro. I have a
> different request: I think that there should be a macro which rather than
> relying on a naming convention allows you to explicitly name the parser that
> implements the rule. This would allow people to use their own conventions.

This is easy for me to add, but it's also really easy for you to
write. I find such low-effort macros unappealing as a library
feature, since they're so easy for the user to use, if desired.

> I tried using nocase and discovered that the symbols parser ignores it. Is this
> by design? If so, there should be a separate parser that allows case-insensitive
> symbol matching.

Nope, that's just an oversight. I went through all the parsers one by
one, but the symbol table is its own thing, and got missed. Ticket:

> Finally, several people asked for benchmarks. Conveniently, the library
> includes a JSON parsing example, and conveniently I am maintaining Boost.JSON
> and I know how to add new parser implementations to its benchmark runner. The
> results aren't very good: Boost.JSON with default allocator is approximately
> 500 times faster than Parser on apache_builds.json. Niels Lohmann's JSON
> library is 100 times faster.

Ha! That's hilariously bad. I'll have to take a look at what might be
going on there. Did you happen to take a look at the callback JSON
parser for comparison?

> * The main requirement is to fix build failures on popular C++ implementations.
> Related to this would be a well-functioning CI setup.
> * Either symbols should support case-insensitive mode, or an alternative parser
> should be added to the library.
> * There should be a rule definition macro that allows explicitly specifying
> parser that implements the rule.
> Overall, I enjoyed my experience with the library. Given that Spirit is being
> sunsetted, Boost needs a replacement.

I fully agree with the first two. I'll probably only do the last one
if Marshall approves the library, and makes me.


Boost list run by bdawes at, gregod at, cpdaniel at, john at