On Sun, Nov 24, 2019 at 5:00 PM Joaquin M López Muñoz via Boost-users < boost-users@lists.boost.org> wrote:
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
- 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