Hi all, this is just a reminder for all interested people to possibly submit a review for the Fixed Strings library written by Reece Dunn. The review is half over and the response we've got by now is far from sufficient to allow to make a proper judgement. So please try to find some time and write about your findings here. Please find the original announcement below. Regards Hartmut Review Manager --------------------------------- Hi all, The review of the proposed Boost Fixed Strings library written by Reece Dunn starts today (January 19th, 2006) and ends January 28th, 2006. You can download the library here: http://tinyurl.com/czrh9 (http://boost-consulting.com/vault/index.php?action=downloadfile&filename=fi xed_string.zip&directory=Strings%20-%20Text%20Processing&) --------------------------------------------------- About the library: The fixed_strings library is a buffer-overrun safe version of using fixed-size character array data buffers. It provides integration with the C API allowing strcpy, et. al. to be used safely for a fixed string and provides a basic_string interface allowing easy migration to C++ strings. There is also a formatter function object allowing (w)sprintf to be used in a single line! Example: boost::fixed_string< 5 > hi; strcpy( hi, "Hello World" ); hi.push_back( '!' ); std::cout << hi << std::endl; std::cout << boost::formatter( "Meine %s Welt!", "kleine" ) << std::endl; output: Hello Meine kleine Welt! --------------------------------------------------- Please always state in your review, whether you think the library should be accepted as a Boost library! Additionally please consider giving feedback on the following general topics: - What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With what compiler? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain?
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. *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? c:\boost\boost_1_33_0\boost\fixed_string\fixed_string.hpp(602) : error C3861: 'assign': identifier not found, even with argument-dependent lookup. boost::fixed_string<5> fs ("Hello"); // Assume can do this from tests c:\boost\boost_1_33_0\boost\fixed_string\fixed_string.hpp(390): error C3861: 'buffer_': identifier not found, even with argument-dependent lookup *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. Couldnt there be a requirements and effects section for each string operation?. Details of formatter spec in documention? *Design * I would prefer a lot more detail on the functionality of fixed_string. What happens on overrun for example? Is it mentioned? 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? 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? 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 What happens between fixed_string and std::string. Are they compatible? Example code would help to show a few scenarios. *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. regards Andy Little
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
"Reece Dunn" wrote
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!
Hi Reece . I copy pasted the <libs/fixed_string/example/cstring.cpp> into VC7.1 project. I had (as usual) test project set to debug mode. In which case I get the error c:\boost\boost_1_33_0\boost\fixed_string\fixed_string.hpp(390): error C3861: 'buffer_': identifier not found, even with argument-dependent lookup I just tried it in release mode and it works ok. debug settings are /D "WIN32" /D "_DEBUG" /D "_CONSOLE" /FD /EHsc /MTd /Za /Zc:wchar_t /Zc:forScope /GR /Fp".\Debug/Test.pch" /Fo".\Debug/" /Fd".\Debug/" /W3 /nologo /c /ZI /TP FWIW release settings ( works ) /D "WIN32" /D "NDEBUG" /D "_CONSOLE" /FD /EHsc /MT /Zc:wchar_t /Zc:forScope /GR /FAs /Fa".\Release/" /Fo".\Release/" /Fd".\Release/" /W3 /nologo /c /TP I'll look some more tonight . I prefer that to reading documentation . regards Andy little
Andy Little wrote:
"Reece Dunn" wrote
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!
Hi Reece . I copy pasted the <libs/fixed_string/example/cstring.cpp> into VC7.1 project.
I had (as usual) test project set to debug mode. In which case I get the error
c:\boost\boost_1_33_0\boost\fixed_string\fixed_string.hpp(390): error C3861: 'buffer_': identifier not found, even with argument-dependent lookup
I just tried it in release mode and it works ok.
Good :).
debug settings are
/D "WIN32" /D "_DEBUG" /D "_CONSOLE" /FD /EHsc /MTd /Za /Zc:wchar_t /Zc:forScope /GR /Fp".\Debug/Test.pch" /Fo".\Debug/" /Fd".\Debug/" /W3 /nologo /c /ZI /TP
FWIW release settings ( works )
/D "WIN32" /D "NDEBUG" /D "_CONSOLE" /FD /EHsc /MT /Zc:wchar_t /Zc:forScope /GR /FAs /Fa".\Release/" /Fo".\Release/" /Fd".\Release/" /W3 /nologo /c /TP
I'll look some more tonight . I prefer that to reading documentation .
Try the /Za flag that is in the debug version and not the release :). It seems therefore that the review version has a problem when the MS extensions are disabled (the /Za flag). Hmm, I'll see if the two-phase lookup patch fixes the issue. Thus, instead of having: buffer_( ... ); replace that with: this->buffer_( ... ); - Reece
"Reece Dunn" wrote
Andy Little wrote:
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
Is there a way to change this behaviour. I would have expected that it would throw an exception on overrun. In a lot of cases the end result of truncating the string is going to be an error, so I'm surprised that isnt an option. OTOH I see that one of the uses is to prevent buffer overrrun at the hands of malicious users so I can see the reason for the policy. The problem with this is that (as I understand it) the fixed_string isnt actually fixed in size, so its a bit more susceptible to malicious users than a string with const size. Nevertheless I think that contrasting how fixed_string and a char array withstand attacks at the hands of malicious users would make a good useage example. That alone would be a good reason for its existence. [...]
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.
I'm confused as to why fixed_string needs the size template parameter at all if its derived from a variable capacity string and can be resized. The main advantage of the size template parameter would be speed I guess, as well as offering good optimisation opportunities to the compiler because of its fixed size, but some of that would surely be lost as soon as the size is changeable. OTOH I may have the wrong end of the stick, however the documents dont really help me much. As I understand it if I declare fixed_string<10> fs; then some time down the line fs might actually have a capacity of 20 characters. Is that correct? If so I would say just simply do away with the size template parameter altogether as its downright confusing. Either that or make fixed_string have immutable size.
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.
I'm trying to understand why I dont have a sequence of characters in one object and a means of formating it in another.
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;
But why cant you do : fixed_string_no_format_policy str; sprintf <format_message_policy >( str , "Meine %1!s! Welt!", "grosse" ,); etc. IOW separating the formatting functionality from the data in the string? OTOH in c-style strings the text of a particular string is intimately bound up with the format, hence a string needs the format as a policy. Is this the point?
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.
I was thinking of e.g fixed_string<7> fs = "Hello "; std::string str = "World"; str + fs ; fs + str; etc It occurs to me that I'm probably missing the point of fixed_stringsomewhat . Maybe It would help if I was walked through the functionality with some more examples in the documentation.. regards Andy Little
participants (3)
-
Andy Little -
Hartmut Kaiser -
Reece Dunn