The Boost formal review of Krystian Stasiowski and Vinnie Falco's
FixedString library starts today
November 25 and extends up to December 4.
[snip]
Review questions
Here are some questions you might want to answer in your review:
- What is your evaluation of the design?
A drop-in replacement for std::string should not add elements to the design (string_view_type, return type of substr(), etc.), as users will come to depend upon them, and then if they want to switch back to std::string, their code will not compile. I personally do not care about the drop-in replacement goal, but adding things to the design not present in std::string subverts that somewhat.
Throwing when the capacity would be exceeded is wrong. The distinction between a failure mode that should throw and one that should assert/invoke UB is that the exception allows the user to recover, and the assert simply informs the user that their code is broken. Since the capacity is known statically, and since there is no way to change it at runtime, reporting this as a recoverable error, rather than a programmer error, is inappropriate. I know this is at odds with the drop-in replacement goal, but using a type that has at most N characters of stack in place of a type that can use nearly all available heap will never truly be a drop-in replacement. Besides, that ship sailed when you changed the return type of substr().
Could you explain why the fixed_string API is not constexpr? Is it for C++11 compatability? Could you also explain how this meets the constexpr use case goal?
Would it be unreasonable to add op+ back in for operands of the same type? There's no difficulty determining the size of fixed_string<8>("foo") + fixed_string<8>("bar"), right? That's no different from fixed_string<8>("foo") + "bar", which *is* supported.
If capacity() were constexpr static, you would not need static_capacity.
I prefer the name "static_string", in keeping with the long-established "static_vector" from Boost.Container.
to_fixed_string() takes any integral type. This is wrong. Here are the overloads for to_string() from the standard:
string to_string(int val);
string to_string(unsigned val);
string to_string(long val);
string to_string(unsigned long val);
string to_string(long long val);
string to_string(unsigned long long val);
string to_string(float val);
string to_string(double val);
string to_string(long double val);
Note the presence of to_string() overloads for float, double, and long double. Note the conspicuous absence of overloads for char, char8_t, char16_t, and char32_t. This is intentional, and to_fixed_string() should follow this. It's fine to use the current implementation of to_fixed_string() as a common implementation for the integral overloads above, but you should not accept the character types, and should accept the floating point types above.
- What is your evaluation of the implementation?
Please consider adding BOOST_ASSERTs of preconditions. For instance, back() may access a character at an index >= size(), but < capacity(). This will never be caught by ASan or Valgrind, and will not segfault, because a random char within an array of char is always ok to read, within the lifetime of the array. An assert will let users find those errors *much* more easily.
Will the optional dependency on Boost become a permanent dependency on Boost if the library is accepted? BOOST_FIXED_STRING_ASSERT() and friends are confusing to those familiar with Boost implementations.
to_fixed_string() has a constrained declaration, and static_asserts that the constraint is satisfied inside the body. It seems that the static_assert is just noise.
This appears quite wrong, even if Augustin is the one behind it. Did he say why he expects that to work? It looks like it accepts doubles as iterators, for instance.
// Because k-ballo said so
template<class T>
using is_input_iterator =
std::integral_constant<bool, ! std::is_integral<T>::value>;
In the tests:
There is only one iterator type used to test the insert() overload that takes a pair of iterators. This should exercise more types. There is also no test that covers the compile-failure cases (inserting a pair of ints, doubles, etc., should not work). The same thing applies to replace(), and perhaps more iterator-pair overloads. I could not find if or where the iterator-pair constructor was exercised.
Similarly, there is code that exercises the conversion of some type T to string_view_type, but there are no compile-fail tests for this. I don't know if it's that necessary to test the constraint in this case, since the constraint is just std::is_convertible<...>, but in general constrained functions need to be tested in the negative sense (that the correctly reject certain types).
- What is your evaluation of the documentation?
Motivation:
"A dynamically-resizable string is required within constexpr functions." -> "A dynamically-resizable string is required within constexpr functions (before C++20)." (since std::string is usable in a constexpr context in C+20 and later).
Iterators:
You forgot to note that increasing the length of the string will never invalidate iterators, since those operations never change the capacity.
All of the links to source code in the synopses give me 404s.
I feel like the documentation that exists, but defines exactly the same functionality as std::string (e.g. erase()), should be documented via reference to std::string. The fact that all these APIs exist within the fixed_string docs means I cannot easily find the variations from std::string. For instance, I know that any allocating function must throw if the capacity is exceeded (from the intro), but I don't know if there are other variations I need to know about without reading the API docs. With no indication where those variations might exist, I end up needing to read all the API descriptions, most of them identical to what I already know std::string to do, in order to find them. Please short-circuit this for me by only supplying the varying API docs, and saying "the rest is what std::string does, except that the capacity is fixed at N". Of course, if you change throws to asserts in the ways I suggested above, you'll need to note that as well.
If you're going to replicate all the API docs from std::string, please don't leave out the type constraint documentation. For instance, fixed_string(T const& t, size_type pos, size_type n); has a constraint on "T" that you do not document.
You may want to briefly state why it is insufficient just to use a std::string with a fixed-capacity allocator.
I did not initially look a the definition of iterator and const_iterator, but once I did I was happy to see that they were both plain pointers. It's really useful to know that the iterator type is guaranteed to be a pointer, and calling that out explicitly would help users.
- What is your evaluation of the potential usefulness of the library?
Very, based on the usefulness of boost::container::static_vector in contexts in which the maximum number of elements is known.
- Did you try to use the library? With what compiler? Did you have
any problems?
I did not try to use it, simply because I know how std::string works. :)
- How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?
I spent about 3 hours reading the documentation and implementation.
- Are you knowledgeable about the problem domain?
Yes. In particular, I have implemented multiple string-like types.
And finally, every review should answer this question:
- Do you think the library should be accepted as a Boost library?
Yes, with these conditions:
- the to_fixed_string() overloads are changed to match the std::to_string() overloads;
- that detail::is_input_iterator is explained or fixed; and
- compile-fail tests are added to verify that those functions with constraints are correctly constrained.
Zach