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

From: Reece Dunn (msclrhd_at_[hidden])
Date: 2006-01-20 08:38:40

Martin Adrian wrote:
>Sorry to start with a negative review:

It's ok. It'll lead to a better library :).

>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.

The use of the fixed_string_impl class is to save an implementor of a 
basic_string-style class implementing all of the basic_string functions. 
Really, this should be more exposed and part of a set of basic_string helper 
classes, like the iterator implementation helpers are.

The format policy is to help provide support for printf-style functions.

As for overrun_policy, the approach I have taken is to clip the string when 
this occurs. However, it may be useful to have a policy that throws when the 
buffer limit is exceeded.

>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.

This was to get around the problem of needing to do something like:

template< int n >
int strlen( const boost::fixed_string< char, n> & str )
{
   return str.length();
}

so you could write that as:

int strlen( const char_string & str )
{
   return str.length();
}

>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_;
>};

Yes. That would be better, as you could then use whichever variant you 
wanted.

NOTE: In order to use the basic_string functions on both of these (one of 
the requirements of the library), they will both need to be derived from 
basic_string_impl (or something similar).

>I also miss a few things:
>
>- operations/construction with basic_string or boost::range

They would be useful. But then what about others. How about interacting with 
the const_string class or a copy-on-write string, a std::vector< CharT >, 
...? I suppose you could do something like:

template< typename StringT >
void assign( const StringT & str )
{
   assign( boost::begin( str ), boost::end( str ));
}

>- 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

And use strlen() to calculate the length? Then you would miss things like 
this: "Text Document\0*.txt\0\0\0".

>     * let buffer() invalidate the length and update it when needed.
>     * let buffer() return a proxy that sets the length when destroyed.

Both of these solutions seem promising.

>- fixed_stringstream.

This would be a fixed_string_buffer, possibly making use of Johnathan's I/O 
stream library.

>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.

Ok. I'll see if I can come up with a better organisation.

>"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.

I'll make this more explicit in the documentation.

>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

This is being called when you call setlength(). I most likely removed this 
during the modifications since your version. I will readd it and update the 
tests to cover the buffer(), setlength() and other misc. functions.

Thanks for your feedback,
- Reece