Re: [boost] [Review] Fixed Strings library

From: Reece Dunn (msclrhd_at_[hidden])
Date: 2006-01-25 04:35:22

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