[fixed_string] Gavin's review

From: Gavin Lambert (boost_at_[hidden])
Date: 2019-12-04 06:52:21

This is my review of the proposed FixedString library.

- Do you think the library should be accepted as a Boost library?

Yes, I think it should be ACCEPTED into Boost.

Regarding the bikeshedding of the library/class name itself, I prefer 
"fixed_capacity_string", as the most accurately descriptive name I can 
either think of myself or that I have read posted by others.

I don't think we should be afraid of longer class names, even for 
vocabulary types.  People can always alias them to something shorter to 
save typing.

(Second choice would be "array_string", although that risks 
misinterpreting as "string_array", which is something entirely 
different.  "stack_string" is another possibility, though might be 
misleading if contained within another object on the heap.)

- What is your evaluation of the design?

Overall, looks good.  I agree with the motivations for creating such a 
type in the first place.

I would prefer if it didn't reinvent some existing storage concepts 
already provided by boost::static_vector (and I don't entirely buy the 
"reduce dependencies" arguments); however there are some other 
justifications for this that I do consider valid (such as static_vector 
not being constexpr).

As previously noted, I think implementing substr to return a string_view 
is a mistake; this will cause lifetime problems if someone tries to 
drop-in replace a std::string in existing code (which was one of the 
motivating use cases).

I'm happy with this being implemented as a new method "subview".  (Maybe 
std::string should do the same thing...)

However I still disagree with the choice to also provide "substr" but 
with a fixed_string return type.  Its semantics (with regard to the 
capacity of the returned string) are still sufficiently different from 
std::string as to make its usage confusing and poor-performance-prone.

With the method omitted, someone attempting a drop-in-replacement will 
get an error and will need to decide how best to handle it (usually by 
using subview, but this may require other changes to extend lifetime, 
which is why spelling subview as "substr" was bad).

And if they really want a copy of the substring, they can still do that 
via subview simply by assigning it to a fixed_string variable -- this 
would also require them to explicitly choose an appropriate capacity, 
which I regard as a good thing.

For similar reasons to the above, my preference is also to entirely omit 
operator+ (as in the current design).  The consumer can decide how to 
rewrite things to use operator+= instead.

(While there are ways to implement operator+, as has been discussed, all 
of these come with hefty caveats and I don't think a compromise is 
sufficiently safe.)

- What is your evaluation of the implementation?

I did not have time to do more than glance at the implementation.

Although I did previously note (thanks to Deniz for calling attention to 
it) that the unconditional #define BOOST_FIXED_STRING_USE_BOOST in 
config.hpp is not the correct usage for configuration defines in Boost.

I also think that this could make better use of Boost.Config's existing 
defines -- while a stated goal was to also produce a standalone library, 
this could be done by using Boost.Config inside of Boost and using a 
standalone mini-config header with equivalent defines only for the 
standalone build.

I'm also dubious about the division of implementation between three 
identically-named header files.  Unless this is required for 
avoiding-ugly-docs reasons I think it would be preferable to merge these 
into a single header -- and perhaps only a single header, where 
Boost.Config is used.

Finally, though, I wonder if perhaps a variation of this type should be 
implemented which has as storage only the array itself without a 
separate size member.  This might better serve some serialization or I/O 
scenarios (as a drop-in replacement for a raw char array), although with 
the caveat that it would probably have to rely on strlen-equivalents and 
thus could not contain embedded nulls (which is why it should be a 
separate type, if implemented, as this is a larger departure from 
std::string).
   As this would also typically have a smaller capacity than other use 
cases it could do things like zero-init which might be too expensive for 
the general-purpose fixed_string, but which can be very helpful for 
serialization scenarios.

- What is your evaluation of the documentation?

The documentation is very short, but seems sufficient for the small 
scope of the class.

- What is your evaluation of the potential usefulness of the library?

I think it will be extremely useful.

I do wonder if an immutable_string type would also be useful (especially 
for constexpr), but that could be a separate library.

- Did you try to use the library?  With what compiler?  Did you have any 
problems?

I did not try it.

- How much effort did you put into your evaluation? A glance? A quick 
reading? In-depth study?

A quick reading.  Probably a couple of hours in total spread over the 
entire review period.

- Are you knowledgeable about the problem domain?

Yes, I frequently work on code where heap allocation is constrained or 
disallowed.  And with strings.