On Sun, Dec 15, 2019 at 12:53 PM Joaquin M López Muñoz via Boost 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!!

On Sun, Dec 15, 2019 at 12:53 PM Joaquin M López Muñoz via Boost wrote:

10. to_fixed_string should be a set of overloads mathing those of std::string.

What's wrong with one or two function templates constrained on say, integers and floating point numbers?