Andy Little wrote:
Fixed String review - Andy Little
*Usefulness*
The rationale of fixed_string seems very good though especially the C-style interface. I think it would be a useful boost library. Formatter seems to be a useful feature. Users regularly ask for C style string formatting.
Yes. Those were two of the aims of the library.
*Did you try the library. With what compiler.*
Unfortunately wont compile on VC7.1:
boost::fixed_string<5> fs = "Hello"; // Can you do this . If not then why not?
Strange! I am using VC7.1 and get: bash-2.05b$ bjam release msvc-7.1 ...found 32 targets... ...updating 2 targets... msvc.compile.c++ ..\..\..\..\..\..\build\drop\boost-libs\fixed_string\libs\fixed_string\example\msvc-7.1\release\cstrings.obj cstrings.cpp msvc.link ..\..\..\..\..\..\build\drop\boost-libs\fixed_string\libs\fixed_string\example\msvc-7.1\release\cstrings.exe LINK : warning LNK4089: all references to 'MSVCP71.dll' discarded by /OPT:REF ...updated 2 targets... Where I modified cstrings.cpp to have: 13 - boost::fixed_string< 15 > cstr; 13 + boost::fixed_string< 15 > cstr = "Hello";
boost::fixed_string<5> fs ("Hello"); // Assume can do this from tests
Yes. I have been using the tests as the basis for modifications, to hopefully avoid regressions. 1. What compiler settings are you using? 2. Have you tried biulding the tests and the examples? 3. What version of the library are you using? I have tested the most recent version on msvc-7.1 and msvvc-8.0 (release builds), but the older ones were tested with GCC (an older version), Metrowerks CodeWarior and Intel compilers. Note that the review version of the library doesn't work with two-phase lookup, but I have a patch to fix this.
*Documentation*
Sorry documentation is not very good at all. Starting with implementation stuff in boost::detail is off-putting, makes no sense. If it is interface then shouldnt be in detail namespace IMO. Is any implementation stuff in docs really necesssary? Interested users can look at headers anyway I think requirements is wrong name for the Requirements section as requirements has different connotation. Features or rationale might be better. I would like some useage examples early in the documentation. Same for formatter examples.
I am going to collate all the comments related to the documentation and then rewrite it.
*Design *
I would prefer a lot more detail on the functionality of fixed_string. What happens on overrun for example? Is it mentioned?
On overrun, the string is truncated to prevent the overrun happening. It is mentioned in the docs, although not explicitly. Something like: fixed_string< 5 > str = "123456789"; std::cout << str << std::endl; // output: 12345
In Requirements section it states that fixed_string must be direct replacement for char array. What does that mean exactly. Does that include run time performance or utility . Run-time performance would seem to be an important goal anyway. Has this been achieved both in terms of e.g copying speed and in terms of code size, else what are the tradeoffs?
It means that you can take any (or at leasy most) of the C/C++ code that has the form: char buffer[100]; // can replace with boost::fixed_string< 100 > buffer; strcpy( buffer, some_unsafe_usage ); In this version, there will be some performance overhead as mentioned in the initial review to allow operations on any sized fixed string buffers. I haven't done any performance comparisons, so I don't know the impact of my current design. However, the reviewer gave an outline for an improved design in this respect so that if you use the fixed string version (not the any size version), it will perform well. There may also be size/speed penalties because of the additional overrun protection.
What is meant by variable capacity for a fixed_string<N>?. The template parameter would suggest a fixed capacity. Does variable capacity require per string state?
An early version of the library just used fixed_string< N >, so if you wanted to write a function that operated on *all sizes* of fixed_strings, you would need to do: template< int n > int strlen( const fixed_string< n > & str ) { return str.length(); } however, one of the early commenters of the library during development wanted to do that without templastizing the function for the size. Therefore, you can now write: int strlen( const char_string & str ) { return str.length(); } where char_string is a typedef for the variable-capacity base class that fixed_string< n > uses.
Formatter. Why does formatter need to be a policy? Cant it be a separate entity to fixed_string? What is relation (if any) to boost::format. Maybe though fixed_string formatter is another library to fixed-string? Could fixed_string use boost::format interface? Documentation doesnt seem to have details on formatter input
The format_policy contains a vsprintf-style method called format that you can replace with whatever you want. The library provides the C API version, but you could replace this with the Win32 API version, or FormatMessage, for example. The fixed_string class uses this to create a format method, so you can call this directly on the fixed_string object. It uses this method to implement the sprintf functionality. This is not related to boost::format, but printf. If you create a format policy for FormatMessage, you could have something like: std::cout << format_message( "Hello %1!s! World!", "fixed_string" ) << std::endl; boost::fixed_string< 100, char, format_message_policy > fmt; sprintf( fmt, "Meine %1!s! Welt!", "grosse" ); std::cout << fmt << std::endl;
What happens between fixed_string and std::string. Are they compatible? Example code would help to show a few scenarios.
As far as I know, they are not, just as basic_string< char, nocase_traits< char > > is incompatible with std::string! I was not sure what to do here, so I went with the standard. There are two compatibility issues here: 1. compatibility with std::basic_string; 2. compatibility with flex_string/basic_string_impl.
*Vote*
I think the documentation needs to be rewritten with more detail. I'd like to be able to test the library too. A list of compilers that it has been tested on would help. Because of these things I vote no, but I'd be very keen if the documentation was improved and could compile. Sorry I hadnt time for a more detailed review.
Can you try my suggestions above because I have it working with VC7.1 and would like to know why it is not working for you! Thanks for the review, - Reece