Re: [boost] [Review] Formal review of Fixed Strings library starts today

From: Martin Adrian (adrianm_at_[hidden])
Date: 2006-01-20 06:30:39

Sorry to start with a negative review:

What is your evaluation of the design?
--------------------------------------

In the introduction it says "The boost::fixed_string class is aimed at solving 
the problem outlined above and is designed to be a direct replacement for 
character buffers, operating as a fixed-capacity string."
("problem above" is buffer overrun and null termination of c-strings)

With this in mind I think the fixed_string design is very complicated. Where 
in the aim does the need for a fixed_string_impl class and a configurable 
format_policy arise? The more obvious "overrun_policy" isn't in the design.

I also don't like the fixed_string_base class. Deriving fixed_string from 
fixed_string_base means:

1. all fixed_strings have an overhead of 1 pointer and 2 size_t.
2. all operations on fixed_string use one extra pointer indirection.
3. fixed_string can't be used in shared_memory or binary serialization.

I think it would be better with a separate fixed_string_ext friend class that 
works on an external buffer.

template <typename CharT>
struct basic_fixed_string_ext {
  template <size_t N>
  basic_fixed_string_ext(const fixed_string<CharT, N>& f)
    : buffer_(f.buffer()), capacity(N), size_(f.len) { };
  public:
    CharT* const buffer_;
    const size_t capacity_;
    size_t& size_;
};

I also miss a few things:

- operations/construction with basic_string or boost::range

- Easier way to handle length. Currently it is easy to forget to update the 
length after using buffer(). Most of my code looks like
    fstr.setlength(apifunc(fstr.buffer()));
  or
    apifunc(fstr.buffer()); fstr.setlength()
  
  The solution is probably to either:
    * Don't store the length
    * let buffer() invalidate the length and update it when needed.
    * let buffer() return a proxy that sets the length when destroyed.

- fixed_stringstream.

What is your evaluation of the documentation?
-----------------------------
The documentation looks good but I think it is done the wrong way. The main 
interface class "fixed_string" is deep inside the design page and only got 2 
lines of documentation.

"This is the main class that provides the fixed-capacity string 
implementation, deriving itself from boost::fixed_string_base. It also 
provides the constructors specified in the std::basic_string interface."

You need to look in the documentation for fixed_string_base to find out that 
fixed_string got the full basic_string interface plus a few more functions.

What is your evaluation of the potential usefulness of the library?
-----------------------------
Could be useful in applications where you want to avoid dynamic memory 
allocations but I don't find it useful for char array replacement. It is just 
to much trouble with setlength and conversion to/from basic_string.

Did you try to use the library? With what compiler? Did you have any problems?
-----------------------------
I am currently using an earlier version (December 2004). The review version 
doesn't work for me on VC70.

boost\fixed_string\fixed_string.hpp(400) : error C2065: 'recalc_' : undeclared 
identifier

Please always state in your review, whether you think the library should be 
accepted as a Boost library!
-----------------------------
I vote No.