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