[review][fixed_string] Andrzej's feedback

From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2019-12-04 14:14:19

Hi Everyone,
This can hardly be called a review, as I didn't look thoroughly at the
implementation. (I only checked what happens on the resize() over the
capacity(), as this was not clear from the docs.)

I think that ultimately the library would be a useful addition to Boost,
however I would not like to rush it before its design is fully baked, and
my impression is that it isn't yet. Therefore my recommendation would be to
REJECT the library at this point, and encourage the authors to resubmit it
for the second review once the design goals and design decisions have been
clearly settled and communicated.

My vision of a review is that we:
1. First evaluate the design goals, whether they are worth pursuing
2. Then we evaluate the design *against* those goals.
3. Then we evaluate the documentation and implementation *against* the
design and goals.

I cannot even go to #3, as to me #1 is missing some parts.

There are different possible goals:
1. Treat this new string as many other STL sequence containers with
variable size. In particular, users use it as if its max_size() were
infinite, and are prepared that at some point an exception might be thrown.
2. Treat it as a "drop-in replacement" for std::string with no-heap-alloc
guarantee.
3. Make something suitable for embedded systems where the user provides the
guarantee: "I will never make the size() of this string go above this
value. If I did, it would be plain clear that the program is not doing what
it was supposed to."

These goal 3 is in clear conflict with either 1 or 2. This becomes clear
when we consider what should be the response to doing a resize() over
capacity(): a precondition violation or a normal course of action signaled
by an exception.

The authors should make a choice which of these goals is not supported and
communicate it clearly from the outset in the docs. I think this decision
has already been made: the library was extracted from Boost.Beast and there
the purpose must have been clear. It is just that it is not communicated in
fixed_string library.

Any decision is good. In C++ performance trade-offs are part of the
contract (interface), so there is room in Boost for many strings: small
strings, short codes, fixed size strings, fixed capacity strings. But for
each library it should be clear what trade-offs were chosen and why.

We have a situation that there exists static_vactor in Boost, which defines
the resize() over capacity() to be a precondition violation. And
fixed_string needs to provide some response to this fact: why did you
choose a different design?

I consider the choice of name to be part of the design: you cannot assign
the name before you have decided on the goals. "static_string" might be a
good name if you chose the same trade-offs as static_vector. But if you
make different trade-offs, it might be better to use a different name.

Making a resize() over capacity() a precondition violation is a *feature*
useful for bug detection and I do not consider it a valid argument that
"library will throw exceptions and if you never resize() over capacity()
you will never see exceptions or std::abort()". If this is a precondition,
then I expect of a library to put some BOOST_ASSERT() or
_builtin_unreachable() in those paths to enable better bug detection.

I find the goal "a drop-in replacement for std::string" invalid. You surely
mean only a drop in replacement for some subset of usages. I guess this
subset comes from the usage in Beast, and I would expect that it is clearly
outlined what this subset is. One of the operations on strings that I use
most often is operator+, so when I hear that this operation is not
implemented in a class that claims to be a drop-in replacement for string,
I see a contradiction. There might be valid reasons not to provide
operator+, but the documentation cannot state that this is a drop-in
replacement for std::string.

I think that only after the above issues have been sorted out can we have a
review of the implementation and the documentation.

One remark about documentation is that I was not able to find what
`resize()` over `capacity()` does. This is the most important information
that I was after. anything else is more less intuitively clear from the
analogy to std::string.

I hope that this feedback doesn't sound discouraging. I appreciate the work
Krystian and Vinnie have put into making this submission, and I would like
to see this library ultimately included in Boost.

Regards,
&rzej;