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 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>. I spend LITERALLY MILLIONS OF YEARS (definitely literally) programming special __ebco 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