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

From: Matt Borland (matt_at_[hidden])
Date: 2024-02-23 08:58:39

Below is my review of the proposed Boost.Parser. 

## What is your evaluation of the design?

I thought it was easier to pick-up and use than Spirit. How to use the library is clear, and I didn't see any real gotchas floating around. Like others I appreciate that the library has a descriptive name.

## What is your evaluation of the implementation?

It's generally pretty clean. The numeric parsing in <boost/parser/detail/numeric.hpp> seems to be a reduced version of the parser from Spirit X3. To test if it was similar I ran this library through the Boost.Charconv benchmarks for performance evaluation with parsing floats, doubles, uint32, and uint64. I ran this using Homebrew GCC 13.2.0 on an M1 Mac. The test set consists of a test set of 2'000'000 random numbers of each type, and that test set is run 5 times. The timing in the left column is the average time to parse 1 number (i.e. total time / 10'000'000).

----from_chars float----
  29.7 ns | std::strtof float scientific
  36.7 ns | std::strtod double scientific
 519.9 ns | Boost.lexical_cast float scientific
 574.0 ns | Boost.lexical_cast double scientific
  28.3 ns | Boost.Spirit.Qi float scientific
  34.7 ns | Boost.Spirit.Qi double scientific
  72.1 ns | Boost.Parser float scientific
  78.9 ns | Boost.Parser double scientific
  22.0 ns | std::from_chars float scientific
  22.2 ns | std::from_chars double scientific
  23.0 ns | Boost.Charconv::from_chars float scientific
  22.7 ns | Boost.Charconv::from_chars double scientific
  65.6 ns | Google double-conversion float scientific
 115.2 ns | Google double-conversion double scientific

-----from_chars int-----
  23.1 ns | std::strtoul uint32_t
  26.8 ns | std::strtoull uint64_t
  40.4 ns | Boost.lexical_cast uint32_t
  51.4 ns | Boost.lexical_cast uint64_t
  29.3 ns | Boost.Spirit.Qi uint32_t
  29.4 ns | Boost.Spirit.Qi uint64_t
  53.7 ns | Boost.Parser uint32_t
  66.3 ns | Boost.Parser uint64_t
  11.1 ns | std::from_chars uint32_t
  16.4 ns | std::from_chars uint64_t
  10.0 ns | Boost.Charconv::from_chars uint32_t
  16.3 ns | Boost.Charconv::from_chars uint64_t

The library requires C++17 so it may be worth offloading some of the numeric cases to <charconv> if possible.

The only thing I didn't like seems to be a carry-over from Spirit. Most numeric types have a parser of the form type_ (e.g. float_, int_, etc). ulong_long and long_long are both missing the trailing underscore which is a small enough difference to be surprising and a bit annoying.

I think the CI could be more comprehensive. All of the current testing is run on x86_64. The boost drone has ARM Mac, S390X, and aarch64 all running natively. Boost.CI also has some docker images of other architectures. I concur with Chris about adding CodeCov to the CI. I have added it to a number of boost libraries recently, and it has helped us to find, and fix dark corners of the codebase.

## What is your evaluation of the documentation?

It was well written. I appreciate that it's example driven instead of a pure wall of text. You can clearly do a lot with the library and it leads you into depth at an adequate pace.

## What is your evaluation of the potential usefulness of the library?

The potential usefulness is high. Having a solid and modern parser in boost fills a need that I am sure most of us will have throughout our careers.

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

Yes. I ran everything on an M1 Mac with Homebrew GCC 13.2.0. The only real problem I had was with CMake, and I opened an issue against the library here: https://github.com/tzlaine/parser/pull/98

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

I spent time on and off this week. Read the docs, read through the code, wrote the test program, and opened a PR for an issue I had with CMake as mentioned above.

## Are you knowledgeable about the problem domain?

My domain knowledge is more specific to numerical parsing than general purpose like other reviewers.

## Do you think the library should be accepted as a Boost library?

Yes without condition. Thanks for writing this Zach. Based off the git history it has been a long time coming.

## Disclosure 

I am a staff engineer with the C++ Alliance.

Matt