Re: [review][fixed_string] FixedString review results

From: Vinnie Falco (vinnie.falco_at_[hidden])
Date: 2019-12-15 21:32:36

On Sun, Dec 15, 2019 at 12:53 PM Joaquin M López Muñoz via Boost
<boost_at_[hidden]> wrote:
> Candidate Boost.FixedString is ACCEPTED into Boost

Thank you for your generous investment of time in managing this review.

Although my name is associated with it, I feel that Krystian should be
the principal investigator and therefore, take my comments below as
suggestions not requirements.

> 1. CMakeLists.txt should either be made generic or removed from repo (it
> is currently a specific thing for Visual Studio).

Here I must object, I need a CML that produces a proper Visual Studio
project file because that is my primary workflow. This is going to be
a recurring problem with all of my repositories (Beast, JSON come to
mind and I have several more in the pipeline).

I don't have the knowledge to solve it, but if someone was to submit a
patch that makes the CML work correctly for everyone and continues to
satisfy the needs of my workflow while still being maintainable by me,
I would be more than happy to run with that and port it to my other
projects.

>    * static_string

I regret changing the name hastily in the first place, I should have
left it as static_string because this is consistent with
boost::static_vector.

> 3. constexpr as much as possible, taking into account that, pre C++20
> this requires zero initialization of the data array.
> 4. noexcept as much as possible and, in principle, at least as much as
> std::string.
> 5. Provide hash overloads. Up to the authors if they want to cover
> Boost.Hash as well.

I agree with everything here.

> 6. Determine the exact behavior on capacity overflow.

I only just a few weeks ago saw Boost.Container's solution, committed
recently, which is to make the behavior of overflow a policy. Although
my personal view is that this is an overly complex solution, enough
people want the other behavior that it makes sense to just copy what
Ion did and do the same thing in static_string. This will make
everyone happy and it will be consistent with Boost.Container, thus a
fairly safe and uncontroversial move.

> 7. Make substr return a fixed_string rather than a view, and
> additionally provide a
> view-returning subview member function.
> 8. State clearly on the docs what C++ version is required.
> 9 There are wrong "size() + m > max_size()" checks (they could overflow)
> in the code.
> 10. to_fixed_string should be a set of overloads mathing those of
> std::string.
> 11. There's an issue around some detail::is_input_iterator alias that
> Zach asked be
> clarified.

Yes to all.

> 12. Compile-fail tests should be added to verify that those functions
> with constraints are correctly constrained.

Visual Studio doesn't easily support compile-fail tests, so I would
prefer to see these implemented as SFINAE-based static_asserts
instead, so that each compile-fail test doesn't need to go into a
separate build target.

> *Priority 2*

These are all good areas of research.

> 16. BOOST_FIXED_STRING_USE_BOOST is unconditionally defined within the
> code. The
> authors should clearly state what this is for and let the user configure
> the library to
> use with/without Boost or, else, remove the thing entirely.

The idiom I am using in all of my new libraries, which I think should
be applied identically to static_string is thus:

1. The default configuration is to build a linkable library (i.e. not
header only) and require Boost. For static_string, the linkable part
is not really applicable since the library consists only of templates.

2. Optionally, the user can define BOOST_STATIC_STRING_STANDALONE, and
the files will compile without Boost. They will not include any Boost
headers, but all declarations will still be in the boost namespace.
Users who define BOOST_STATIC_STRING_STANDALONE must still qualify
library identifiers with boost::static_string. The standalone version
of the library may have higher requirements. In this case it would
require C++17 (for string_view).

3. Optionally, the user can define BOOST_STATIC_STRING_HEADER_ONLY,
and the files will compile in header-only mode, where no separate
linker input is needed. This is not applicable to static_string but it
is how I am structuring my other libraries (except for Beast, which I
am leaving alone for now to not break users).

> 17. Source code organization (three equally named files, detal and impl
> folders), raised some eyebrows.

Generally speaking this is the source code organization I use, which
closely resembles how Boost.Asio and standalone Asio are structured,
and this is done so for 1. minimizing the amount of header material
exposed to the documentation toolchain, 2. providing the separation
needed to support the header-only configuration, and 3. Separating
public from private implementation. There's nothing generally wrong
with the equally named files, in larger projects it would be highly
annoying to have to give them all different names, it is much easier
to find the matching files when the names are the same.

For static_string though, which is template-only and rather limited in
how large it can get, putting it in one header file probably outweighs
the benefits of separation. And in fact, advertising "just copy one
file into your project to use this!" is a pretty good selling point
(especially with BOOST_STATIC_STRING_STANDALONE defined).

> Vinnie has indicated they may abandon Doxygen in favor of asciidoc
> to produce better-quality docs.

This is up to Krystian, and I support such a change.

I have no opinions on the rest.

Thanks!!