[FixedString] Deniz' review

From: Deniz Bahadir (deniz.bahadir_at_[hidden])
Date: 2019-11-25 17:55:36

This is my review of Krystian Stasiowski and Vinnie Falco's FixedString 
library.

The important question and its answer upfront:

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

I think it should be ACCEPTED into Boost.

It is a very small (almost trivial) library and in the beginning I was 
not sure if it really should go into Boost. However, for me it looks 
like a useful building-block, so I vote ACCEPT.
And if I remember correctly Vinnie said he is already having some other 
libraries using it which he wanted to propose for Boost later, so there 
seem to be some real use-cases for it already.
Besides, Boost already has some other smaller building-block libraries, 
so FixedString should be in good company.

On a side-note: It would be helpful for the Boost library documentation 
overview (e.g. https://www.boost.org/doc/libs/1_71_0/) to allow more 
views/sort-orders for listing available Boost libraries than the 
views/sort-orders which are already there. (Having "building-blocks" and 
especially "newer C++ standard features for C++03" or similar "standard 
compatibility" categories for the Boost libraries comes to mind.)
OK, now back to the review.

- What is your evaluation of the design?

The overall design looks straight forward.
`boost::fixed_string`'s API resembles that of `std::string` and only the 
internal representation differs largely. This looks like the correct 
design for the aimed purpose of this library (to provide a dynamically 
resizable string with compile-time fixed capacity.)

- What is your evaluation of the implementation?

The implementation looks solid and also straight forward. However, I 
have some remarks to make which you can read further down in this email.

- What is your evaluation of the documentation?

The documentation is quite small, except for the reference section. 
This, however, should be fine considering the very simple and small 
focus which FixedString has.

Regarding the "Iterators" section, it was not clear to me on first read 
for which string type the iterators are invalidated when moving/swapping.
I think, `boost::fixed_string` is meant and not `std::string`, but that 
was not obvious from first read and the wording should probably be 
improved there to be unambiguous. (Maybe make this clear by speaking of 
"FixedString" or "fixed_string" instead of just "string".)

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

For the use-cases mentioned in the "Motivation" section of the 
documentation it seems to be useful. All other use-cases I could think 
of are specializations of the mentioned use-cases.

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

No, I have not tried to use the library.
However, I am curious to know if Boost.String_Algorithms can operate on 
`boost::fixed_string`, too. If not, it would be nice for the code of 
FixedString and/or Boost.String_Algorithms to be modified in such a way 
that they can inter-operate with each other.

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

I read the documentation, but the reference section I merely visually 
scanned through. (I assume, there are no mistakes in it, as they mainly 
resemble the API of `std::string`.)
I looked at the implementation but did not look at all function 
implementations in all depths. I, however, looked at the CMakeLists.txt 
file, too, and have some remarks for it (see below).

Overall, I probably spent about 1.5 hours for the review, not including 
the writing of this email.

- Are you knowledgeable about the problem domain?

Only as far as it gets if you are a general user of `std::string`.

I have the following additional remarks:

* `boost::to_fixed_string` / `boost::detail::max_digits`:

- Is the returned `boost::fixed_string` type of `boost::to_fixed_string` 
large enough to also hold the (plus) sign of an unsigned integer (if a 
user might want to append it later)? (Reading the comment and 
implementation of `boost::detail::max_digits` I assume the answer is 
yes.) Maybe, make this clear in its reference documentation.

* "boost/fixed_string/config.hpp":

- At the top of this file we have `#define 
BOOST_FIXED_STRING_USE_BOOST`, which therefore makes all following 
conditional definitions always use Boost equivalents of standard library 
types internally.
Is it really desired to have it hard-coded here? Shouldn't the user 
decide if s/he wants to use Boost compatibility types instead of the 
ones coming from the standard library?

* Filenames

- I do not really like, that all files have the same name 
"fixed_string.hpp". There is the main one in "boost/fixed_string", 
another one in "boost/fixed_string/detail" and a third one in 
"boost/fixed_string/impl".
I would prefer these files having some (at least slightly) different names.

* "CMakeLists.txt":

- This CMakeLists.txt file is written in Traditional CMake (old style 
CMake) although requiring CMake 3.5.1 which knows how to handle Modern 
CMake. You should consider using Modern CMake instead.

- You should leave `CMAKE_CXX_FLAGS` untouched. It is for the user of 
the library, not for the developer!

- Similar, set requirements for a specific minimum required C++ version 
using the `CXX_STANDARD` property instead of hard-coding it into 
`CMAKE_CXX_FLAGS`.

- You should probably replace usage of `include_directories` with 
`target_include_directories` on the specific targets.

- Similar for `add_definitions` and `link_directories`.

- Using `file(GLOB...)` for globbing sources is dangerous and CMake 
might not always realize that some sources have changed. (see note at: 
https://cmake.org/cmake/help/latest/command/file.html#glob)

- In general, I am not sure if that CMakeLists.txt file should have been 
part of the review, but I recommend to provide a useful and more modern 
one if this is going to become a part of Boost, too.

Best regards,
Deniz