[review][fixed_string] FixedString review results

From: Joaquin M López Muñoz (joaquinlopezmunoz_at_[hidden])
Date: 2019-12-15 20:53:11

These are the results of the candidate Boost.FixedString review. Thanks 
to all the people
(and there's been many) who contributed. To clear up the tension, let me 
begin by
announcing that:

Candidate Boost.FixedString is ACCEPTED into Boost. Acceptance is 
subject to the authors'
addressing of some issues raised during the review that I label as 
"priority 1" below.

*Summary of reviews*

We've had 10 full reviews:

 Â  * Accept: Deniz Bahadir, Emil Dotchevski, Gavin Lambert, JeanHeyd 
Meneide,
 Â Â Â  Paul A. Bristow, Peter Dimov, Peter Koch Larsen.
 Â  * Conditionally accept: Zach Laine.
 Â  * Reject: Andrzej Krzemienski, Phil Endecott.

Zach's conditions to acceptance seem minor and easily addressable to me.
Andrzej based his rejection on a perceived lack of clarity as to what 
the actual pitch of
the library is, which ultimately boils down to the issue of 
throwing/asserting on
capacity overflow: my impression is that a strong pitch, if not a 
universally acclaimed
one, emerged during discussions.
Finally, Phil voted to reject but then went on to state that he would 
accept based
on a number of "minimally sufficient" conditions that are trivial to 
meet (or even
have been already implemented by Krystian).

All in all, I think it is a reasonable compromise to grant acceptance 
based on the
resolution of a number of important issues, listed below.

*Issues*

I've classified issues in three different priorities:

 Â  * Priority 1: to be solved *before* inclusion into Boost.
 Â  * Priority 2: to be addressed somewhere in the future.
 Â  * Priority 3: anecdotal or without clear support from the community, 
to be addressed
 Â Â Â  at the authors's discretion.

* Priority 1 issues*

1. CMakeLists.txt should either be made generic or removed from repo (it 
is currently
a specific thing for Visual Studio).
2. Choose a name for the component. Some consensus arose that naming 
should follow
std::string's and be of the form basic_foo<...> with aliases foo, wfoo, 
etc. As for "foo", the
following candidates have been proposed:
 Â  * static_string
 Â  * fixed_capacity_string
 Â  * array_string
 Â  * static_capacity_string
 Â  * inplace_string
 Â  * capped_string
 Â  * bounded_capacity_string
 Â  * bounded_string
To be clear, I don't think any of these is clearly more popular than the 
rest, so it is up to
the authors to make up their mind.
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.
6. Determine the exact behavior on capacity overflow. I think *some* 
partial consensus
arose around throwing via boost::throw_exception so as to provide an 
opt-out mechanism
for exception-handicapped scenarios. This has been the most contentious 
issue during the
review but I think the authors are in their right to decide on throwing 
much as anyone
can decide otherwise and propose what would be basically a different 
library.
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.
12. Compile-fail tests should be added to verify that those functions 
with constraints are
correctly constrained.

*Priority 2*

13. Determine if fixed_string<0> should be specialized or not to make it 
more memory
efficient. This raises potential issues in the uniqueness of the pointer 
returned by data().
14. Determine if operator+(fixed_string,fixed_string) should be 
provided. My impression
is that people generally wanted this, and I've been tempted to raise 
this to priority 1.
15. Optimize the type of the internal size data member when size_t is 
larger than
necessary.
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.
17. Source code organization (three equally named files, detal and impl 
folders), raised
some eyebrows. Some (including Vinnie) favor a single-file implementation.
18. Docs should be more C++ standard-like and provide a synopsis. Vinnie 
has indicated
they may abandon Doxygen in favor of asciidoc to produce better-quality 
docs.
In general, people were not enthusiastic about the documentation.
19. The current monolithic test should be split up in lighter, more 
manegeable test
files. Peter Dimov suggested some additional improvements on the testing 
part.

* Priority 3*

20. Provide a literal operator""
21. Should traits parameter be removed?
22. Store size (as capacity-size) in the last char of the data array, so 
as to save one
memory byte. This might affect performance, though.
23. Add functions for checking remaining capacity.

Thanks to Krystian and Vinnie for their work! And to everybody who has 
contributed
to this really interesting review.

Best regards,

Joaquín M López Muñoz
Review manager for candidate Boost.FixedString