Re: [review][fixed_string] FixedString review starts today

From: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2019-11-27 16:56:17

On Sun, Nov 24, 2019 at 5:00 PM Joaquin M López Muñoz via Boost-users <
boost-users_at_[hidden]> 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<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