[Parser] Review Results and Report

From: Marshall Clow (mclow.lists_at_[hidden])
Date: 2024-03-15 21:00:21

The review of Zach's parser library ran from Feburary 19 -> 28th.

If I have mischaracerized/incorrectly stated anyone's position,
please let me know in this thread.

REVIEW RESULT
-----------------------------

The result is Boost.Parser is
* ACCEPTED WITH CONDITIONS * into Boost.

The following tickets in the parser repo should be addressed before adding this
library to boost:

Consider changing the way semantic actions are invoked https://github.com/tzlaine/parser/issues/106
Turn on -Wall for GCC and Clang builds https://github.com/tzlaine/parser/issues/107
Clearly indicate which directives create a new parser when used, which do not, and why it matters. https://github.com/tzlaine/parser/issues/161
Enable code coverage on Github project https://github.com/tzlaine/parser/issues/146
Add more examples and use cases to The Parse Context https://github.com/tzlaine/parser/issues/145
ABI-tag __has_include-guarded differing code https://github.com/tzlaine/parser/issues/151
API reference for char_ should explicitly state that it can be used without args https://github.com/tzlaine/parser/issues/150
Add no_case support to symbol tables https://github.com/tzlaine/parser/issues/149

REVIEW REPORT
-----------------------------
We recieved 10 reviews during the review period, and two more after the review ended.

Six reviews recommended ACCEPT
One review recommended ACCEPT WITH CONDITIONS
Three reviews reccommended REJECT
One review did not reccommend an action.
One review was of the documentation only.

Thanks to everyone who participated.
Thanks to Zach for writing the library, and participating heavily in the review.

REVIEW COMMENTS
-----------------------------
Several of the reviews sparked discussions, and many of the issues raised
were debated back and forth on the mailing list.

There was a lot of discussion about the use of Boost.Hana in the library;
Zach defended it on the grounds that Hana's tuple has an `operator[]`, which is
much more readable than using `std::get`.  There was push back from the reviewers
on this, not wanting to add a dependency (and learning curve) for yet another
library.  Currently, this is selectable at compile time; Zach said that he would
consider defaulting to use std::tuple instead of Hana::tuple.

Phil had trouble with a custom parser that used the double parser; I believe
that this was resolved, and this is now captured as a test case:
https://github.com/tzlaine/parser/blob/boost_review_changes/test/parse_coords_new.cpp

There was a long discussion about semantic actions, and how they are defined/invoked.
https://github.com/tzlaine/parser/issues/106 was created to address this.

Several people mentioned compiler error messages
one such example is https://github.com/tzlaine/parser/issues/105

and compiler warnings
https://github.com/tzlaine/parser/issues/107

Christian thinks that the examples comparing Parser to Spirit3 should emphasize
the advantages of Parser.  Zach to update the examples.

He also asked for general design guidance on writing rules vs. actions.
https://github.com/tzlaine/parser/issues/161

Matt (and others) asked for more CI coverage; Zach said that he welcomed help setting it up.
https://github.com/tzlaine/parser/issues/146

Alan requested that the numeric parsers use Boost.Charconv for the conversions.

He also stated that the section in the docs "The Parse context" is confusing
https://github.com/tzlaine/parser/issues/145

David noted some ODR issues
https://github.com/tzlaine/parser/issues/151

and he also noted that the docs for the char parser don't say it can be used
without arguments: https://github.com/tzlaine/parser/issues/150

He also mentioned that he would really rather see a library design more like
Text.Parsec from Haskell.

Дмитрий wanted to know why the symbols didn't support 'nocase'. 
https://github.com/tzlaine/parser/issues/149

He also thought that the attribute for
parsers with semantic actions is none is not advertised well enough
https://github.com/tzlaine/parser/issues/148

Mohammed asked a question about parsing chunk-encoded HTML bodies efficiently.
Zach replied that this wasn't supported directly, but you could write a custom
parser to do it.

Christian reported a hideous performance overhead when parsing JSON. (100x slower
than Boost.JSON)
Zach and Peter investigated, and it turned out that the construction of a
stringstream for tracing was much of it, and the rest was also part of the tracing
mechanism.  Zach reworked the tracing code, and now non-debug builds are much faster.
https://github.com/tzlaine/parser/issues/152

REVIEW ACKNOWLEDGMENTS
-----------------------------

Thanks again to Zach for Boost.Parser

Many thanks to our expert reviewers, commenters and
watchers. Your contributions keep Boost excellent!

Marshall Clow
Boost.Parser Review Manager