[ Review ] [ fixed_string ] Accept

From: JeanHeyd Meneide (phdofthehouse_at_[hidden])
Date: 2019-12-05 01:26:49

Dear Boost,

     This is a review of boost.fixed_string. It is my first review, so
take it with a grain silo's worth of salt.

-------------
Summary 📝
--------------
Accept fixed_string. It's about

----------------
What I Did ⚙️
----------------
- I ran its tests, VS 2019 i686 and x86-64 | VS 2017 i686 and x86-64 |
MinGW 9.2.0 i686 and x86-64
- I put it inside itsy.bitsy (https://github.com/ThePhD/itsy_bitsy)
and used it as an underlying container in its tests.
- I put it inside my private text repository as an underlying
container to hold code units for several different encoding schemes
and it work (https://github.com/ThePhD/text-dev | far-behind
implementation at https://github.com/ThePhD/text).
- I generally messed around with it in a basic main.cpp file and read
the docstrings and docs while comparing it to cppreference.

--------------
Strengths 💪
--------------
+1 - Follows std::string's container design nicely, making it easy to
program around and know what to expect from the outset. (Side note:
ignore whoever said to remove the initializer list ones: I used all of
them. It's necessary to be considered a SequenceContainer
(https://en.cppreference.com/w/cpp/named_req/SequenceContainer).)
+1 - Works with all character types.
     char, wchar_t, char16_t, char32_t, and -- in C++20 mode --
char8_t for the compilers that have it.
+1 - insert, erase, clear, push_back, push_front meet specification
     I flexed it by inserting it as part of itsy.bitsy's tests and
using it as one of its sequence containers.
+1 - docs are straightforward.
+1 - iterators work just fine.
     I tested it using itsy.bitsy as an underlying container for a
reference_wrap'd bit_view as well as a stress testing it under my
phd::text implementation. It worked fine as an underlying container
for UTF8, UTF16. UTF32, and UTF-7-EBCDIC storage.
+5000 - Standalone.
     I wish more Boost libraries were packed this way. I can make it
not depend on Boost as a whole; great for dropping into random
codebases and using as a replacement. Also compiles faster for those
of us who have C++17+ capable compilers.

-------------------
Weaknesses 🤒
-------------------
-1 - Naming.
     fixed_string<500> works, wfixed_string<500> doesn't,
basic_fixed_string<char, 5000> doesn't either. It uses a different
naming scheme from the standard, and led me around in circles on first
use until I forced myself to read the template head of the class
declaration. But it's Boost, so we can afford to be different and
stuff.

     Just don't do static_vector. :D

-0 - operator+
     I wanted it. It wasn't there. I can see why, since you would have
to pessimistically expand the size to the max between the two. Do
enough additions and you can explode the final result's stack size
several, so this isn't really a loss. Besides,-0 is just 0 on any
reasonable 2's complement machine.

-0 - noexcept
     It's 2019, everyone's getting on the noexcept train. Don't let it
leave you behind!

-2500 - Constexpr is lacking.
     I could not compile any of itsy.bitsy's constexpr, compile-time,
or static_assert tests. I could not compile any of the static_assert
tests from latest private text development. This prevents a wide range
of applications, including my failure to substitute boost.fixed_string
for Hana's Fixed String implementation in CTRE and failure to use
fixed_string as a compile-time buffer for encoded and decoded text.

--------------
Misc.
--------------
- I see you added subview, very nice. 👏
- substr should continue to return a fixed_string.
- The empty optimization and size optimization is important. Don't
listen to anyone who tells you fixed_string<0> should report false for
std::is_empty_v<fixed_string<0>>. I spend LITERALLY MILLIONS OF YEARS
(definitely literally) programming special __ebco<T, tag> or slapping
[[no_unique_address]] on things to cut down on memory sizes. Please
don't cause a cascade of bloated size with MSVC's current crappy base
class """optimization""" rules (the new, better ones are still opt-in
because yay, ABI breaks). The closer things are to being small enough
or trivial enough that they get register-passed, the better.
- Speaking of the size optimization, MSVC's standard string also
employs a trick where they shove the size in the last CharT. May be of
use to you here, but probably not worth the efforts.

--------------
Unrelated
--------------

The following is boring things not related to the review but stated
here for extra flavor that can be completely ignored:
- Boy that CMake file hurts my feelings. I fixed it up so I could use
it with add_subdirectory. It promised me an easy integration and
smacked me upside the head instead. But at least I could get the tests
going soon enough. Maybe don't commit it, it gives the impression it's
for public consumption (it's not).
- Tests are slightly inscrutable, better names would be nice.
- For constexpr: use a union of { char derp; CharT value; } and
value-initialize the char. Provide iterators that walk over the union
and on deref directly handle you the CharT& lvalue reference. .data()
is unimplementable as constexpr like this but who cares: you have a
constexpr begin() and that's what people should be using. .data() is
for type-punning shenanigans.
- You should turn up the warnings on MSVC: there's a few places where
there's an implicit narrowing thanks to going from size_type
(std::size_t) to smallest_width_t and MSVC gives a snippy warning.

      Overall, yay! Try to get the constexpr stuff in quick; that's
what's going to make this library interesting to use for C++20 and
beyond, and have Boost libraries start being things to look forward
to, rather than representing the Old, Boring Establishment that I
reach for when I hear the words "Sea Plus Plus Oh Three".

Great Job,
JeanHeyd Meneide