[Review] Fixed Strings review period extended

classic Classic list List threaded Threaded
42 messages Options
123
Reply | Threaded
Open this post in threaded view
|

[Review] Fixed Strings review period extended

Hartmut Kaiser

Hi all,

normally, the review period of the Fixed Strings library written by Reece
Dunn ended today. All we got so far are 3 reviews, which is too less to make
a proper decision. I really would like to encourage you to submit a review
for this library to get a representative result. For this reason I'ld like
to extend the review period for another week until February 5th to allow for
more reviews. I'm convinced, that a Fixed Strings library would make a good
addition to Boost and in the end it should be useful to a lot more of people
than to the 3 of us who send in a review.

Regards Hartmut
Review Manager


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

Andy Little

"Hartmut Kaiser"  wrote

>
> Hi all,
>
> normally, the review period of the Fixed Strings library written by Reece
> Dunn ended today. All we got so far are 3 reviews, which is too less to make
> a proper decision. I really would like to encourage you to submit a review
> for this library to get a representative result. For this reason I'ld like
> to extend the review period for another week until February 5th to allow for
> more reviews. I'm convinced, that a Fixed Strings library would make a good
> addition to Boost and in the end it should be useful to a lot more of people
> than to the 3 of us who send in a review.

A Fixed Strings library but this one? . What interests me is that  the
fixed_string library is way ahead as the top download in the Vault download
list, but few reviewers have resulted.  I feel the mystery can be explained
by the reviews so far. looking through them we have all said pretty much the
same thing. Surprise
that there is no overflow policy. Concern at the lack of detail in the
documentation and the way the documentation and the class itself are laid out.
IOW as currently implemented fixed_string isnt delivering what it seems to
promise. I hope Reece takes this in his stride by the way . I would guess that
has to be the
reason why so many downloads have resulted in so little response. It must be
worth analysing that lack of response as a first step in revisiting the design.

There must be some of  the 421 downloaders of fixed_string that can shed some
light on this?

regards
Andy Little




_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

Hartmut Kaiser
 
Andy Little wrote:

> > normally, the review period of the Fixed Strings library written by
> > Reece Dunn ended today. All we got so far are 3 reviews,
> which is too
> > less to make a proper decision. I really would like to
> encourage you
> > to submit a review for this library to get a representative result.
> > For this reason I'ld like to extend the review period for
> another week
> > until February 5th to allow for more reviews. I'm convinced, that a
> > Fixed Strings library would make a good addition to Boost
> and in the
> > end it should be useful to a lot more of people than to the
> 3 of us who send in a review.
>
> A Fixed Strings library but this one? .

If you read my mail carefully - I never said 'this one'.

> What interests me is
> that  the fixed_string library is way ahead as the top
> download in the Vault download list, but few reviewers have
> resulted.  I feel the mystery can be explained by the reviews
> so far. looking through them we have all said pretty much the
> same thing. Surprise that there is no overflow policy.
> Concern at the lack of detail in the documentation and the
> way the documentation and the class itself are laid out.
> IOW as currently implemented fixed_string isnt delivering
> what it seems to promise. I hope Reece takes this in his
> stride by the way . I would guess that has to be the reason
> why so many downloads have resulted in so little response. It
> must be worth analysing that lack of response as a first step
> in revisiting the design.

As you might have expected, I read through the reviews _very_ carefully.

There are two reasons, why I proposed to extend this review.
1) The library was downloaded by more than 400 interested people and I don't
expect the 3 reviews to represent all of them.
2) Even if the currently reviewed library won't be accepted to Boost mainly
for the reasons stated by the three reviews, I think we need a Fixed Strings
library in Boost and the more dicussion we get the better for a future
library.

Regards Hartmut


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

Zigmar
In reply to this post by Andy Little
On 1/30/06, Andy Little <[hidden email]> wrote:

>
> "Hartmut Kaiser"  wrote
> >
> > Hi all,
> >
> > normally, the review period of the Fixed Strings library written by Reece
> > Dunn ended today. All we got so far are 3 reviews, which is too less to make
> > a proper decision. I really would like to encourage you to submit a review
> > for this library to get a representative result. For this reason I'ld like
> > to extend the review period for another week until February 5th to allow for
> > more reviews. I'm convinced, that a Fixed Strings library would make a good
> > addition to Boost and in the end it should be useful to a lot more of people
> > than to the 3 of us who send in a review.
>
> A Fixed Strings library but this one? . What interests me is that  the
> fixed_string library is way ahead as the top download in the Vault download
> list, but few reviewers have resulted.  I feel the mystery can be explained
> by the reviews so far. looking through them we have all said pretty much the
> same thing.
For me personally, the reason I didn't submit any review, that I'm not
completely understand the purpose of the library. The documentation
(as few mentioned) lacks motivation and general description. So how
can I review the library, if I can't understand what the library is
trying to solve and how. I can't say for others, but for it is the
reason. I suggest that even if documentation can't be updated during
formal review, that the author will give a little more extended
explanation here, at mail list.


--
Best regards,
Zigmar

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

Reece Dunn
In reply to this post by Andy Little
Andy Little wrote:
>A Fixed Strings library but this one? . What interests me is that  the
>fixed_string library is way ahead as the top download in the Vault download
>list, but few reviewers have resulted.  I feel the mystery can be explained
>by the reviews so far. looking through them we have all said pretty much
>the
>same thing. Surprise
>that there is no overflow policy.

I intend to fix this for the next review :).

>Concern at the lack of detail in the
>documentation and the way the documentation and the class itself are laid
>out.

I hope to improve the documentation so that users can follow it better.

>IOW as currently implemented fixed_string isnt delivering what it seems to
>promise. I hope Reece takes this in his stride by the way . I would guess
>that
>has to be the
>reason why so many downloads have resulted in so little response. It must
>be
>worth analysing that lack of response as a first step in revisiting the
>design.

I can only fix issues that are being raised.

That said, I intend to move the basic_string_impl class out of the detail
namespace as this is more useful in other contexts. That class is based on
the flex_string class written by Andrei Alexandrescu. However, this class -
like the basic_string class - is *way* too large.

I have experimented with splitting this class into several smaller ones
based on the grouping in the standard documentation (iterators, capacity,
etc.) so you can combine them. This way, it will be like the iterator
classes. These have had some limited success and I intend on working on this
after fixing the issues raised in the review.

With this change, I intend to move the fixed_string class into boost/string
so that other string classes like const_string can be placed there as well.

- Reece


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

Reece Dunn
In reply to this post by Zigmar
Pavel Antokolsky aka Zigmar wrote:
>For me personally, the reason I didn't submit any review, that I'm not
>completely understand the purpose of the library. The documentation
>(as few mentioned) lacks motivation and general description. So how
>can I review the library, if I can't understand what the library is
>trying to solve and how. I can't say for others, but for it is the
>reason. I suggest that even if documentation can't be updated during
>formal review, that the author will give a little more extended
>explanation here, at mail list.

The main aim of the library was presented in the "The Problem"/"The
Solution" sections (the first ones) of the documentation.

Normally, in C (or even some C++ code), you have constructs that look like
this:

   char buffer[ 15 ];
   sprintf( buffer, "Some %s text", "verly long" );

The problem is that the above would cause a buffer overrun, which is the
most common cause of denial of service attacks and other security holes in
major applications. The variant that allows you to specify the size of the
string buffer is better, but not prefect. Consider the following:

   wchar_t buffer[ 5 ];
   wcsncpy( buffer, sizeof(buffer), L"12345678" );

At first glance, this code looks safe, but will also cause a buffer overrun.
The fixed_string class is designed to solve this problem. The above examples
would be:

   fixed_string< 15, char > buffer;
   sprintf( buffer, "Some %s text", "verly long" );

and:

   fixed_string< 5, wchar_t > buffer;
   wcscpy( buffer, L"12345678" );

If the introduction does not make this clear, I have written the
documentation wrong. *This* is why feedback is useful - to know what people
don't understand about the documentation, so I can address those issues for
the next review. Otherwise, the documentation will most likely still suffer
from the same problems.

Also, the C and C++ string implementations do not prevent you passing in
null strings. Thus, in the msvc 7.1 implementation, the following will cause
a run-time crash:

   std::string foo( 0 );
   std::ostringstream ss;
   ss << "Crash! Boom! Bang!" << static_cast< const char * >( 0 ) <<
std::endl;

- Reece


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

David Abrahams
"Reece Dunn" <[hidden email]> writes:

> Pavel Antokolsky aka Zigmar wrote:
>>For me personally, the reason I didn't submit any review, that I'm not
>>completely understand the purpose of the library. The documentation
>>(as few mentioned) lacks motivation and general description. So how
>>can I review the library, if I can't understand what the library is
>>trying to solve and how. I can't say for others, but for it is the
>>reason. I suggest that even if documentation can't be updated during
>>formal review, that the author will give a little more extended
>>explanation here, at mail list.
>
> The main aim of the library was presented in the "The Problem"/"The
> Solution" sections (the first ones) of the documentation.
>
> Normally, in C (or even some C++ code), you have constructs that look like
> this:
>
>    char buffer[ 15 ];
>    sprintf( buffer, "Some %s text", "verly long" );
>
> The problem is that the above would cause a buffer overrun, which is the
> most common cause of denial of service attacks and other security holes in
> major applications. The variant that allows you to specify the size of the
> string buffer is better, but not prefect. Consider the following:
>
>    wchar_t buffer[ 5 ];
>    wcsncpy( buffer, sizeof(buffer), L"12345678" );
>
> At first glance, this code looks safe, but will also cause a buffer
> overrun.
>
> The fixed_string class is designed to solve this problem. The above examples
> would be:
>
>    fixed_string< 15, char > buffer;
>    sprintf( buffer, "Some %s text", "verly long" );
>
> and:
>
>    fixed_string< 5, wchar_t > buffer;
>    wcscpy( buffer, L"12345678" );


The reason I didn't submit a review was that I didn't have time.  But
more generally, I don't think fixed strings are the right tool for
this job 99% of the time.  A variable-sized string with small string
optimization will serve nicely, and if used properly, will virtually
eliminate the problem of buffer overruns.  I don't think we should be
encouraging people to patch their way around unsafe code in a way
that's still unsafe.

It's still unsafe because the code's original author expects the code
to work, not for it to throw an exception or truncate the input or
whatever the proposed library does.

--
Dave Abrahams
Boost Consulting
www.boost-consulting.com

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

Zigmar
In reply to this post by Reece Dunn
Hi, Reece

On 1/30/06, Reece Dunn <[hidden email]> wrote:
> Pavel Antokolsky aka Zigmar wrote:
> >For me personally, the reason I didn't submit any review, that I'm not
> >completely understand the purpose of the library. The documentation
> >(as few mentioned) lacks motivation and general description. So how
> >can I review the library, if I can't understand what the library is
> >trying to solve and how. I can't say for others, but for it is the
> >reason. I suggest that even if documentation can't be updated during
> >formal review, that the author will give a little more extended
> >explanation here, at mail list.
[...]
> If the introduction does not make this clear, I have written the
> documentation wrong. *This* is why feedback is useful - to know what people
> don't understand about the documentation, so I can address those issues for
> the next review. Otherwise, the documentation will most likely still suffer
> from the same problems.
Unfortunatly, it is not clear. Now, I'm beginning to see the point.
Originally, after the reading introcution, especially first sentence
(
    "There is a lot of C (and even C++) code, that uses character
buffers and the C-style string functions for various reasons: updating
legacy C code, wanting to keep executable size down without linking to
a large dynamic library or wanting to tune performance by keeping the
strings on the stack."
) I've got the idea, that the main purpose it to create _fast_
stack-based strings with C++ interface (something like boost::array),
and buffer overrun protection are just nice feature of such strings.
As far as I understand now from you description, previews reviews and
my look at the code, perfomance is not a main goal at all, and the
buffer protection is the target. But then, why string should be
"fixed" (and, AFAIU, it is not exactly fixed)? Isn't it just, say,
safe_string? I'm not criticizing,  just trying to understand the
rationale...
--
Best regards,
Zigmar

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

Jim Douglas-2
In reply to this post by Reece Dunn
Reece Dunn wrote:
> Normally, in C (or even some C++ code), you have constructs that look like
> this:
>
>    char buffer[ 15 ];
>    sprintf( buffer, "Some %s text", "verly long" );

No self-respecting coding standard would allow you to write the code
above. Rather it would insist that at least you wrote:

     snprintf( buffer, 15, "Some %s text", "verly long" );

Does that not solve the problem of overruns?

Jim

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

Andy Little
In reply to this post by Reece Dunn

"Reece Dunn"  wrote

> Andy Little wrote:
>>A Fixed Strings library but this one? . What interests me is that  the
>>fixed_string library is way ahead as the top download in the Vault download
>>list, but few reviewers have resulted.  I feel the mystery can be explained
>>by the reviews so far. looking through them we have all said pretty much
>>the
>>same thing. Surprise
>>that there is no overflow policy.
>
> I intend to fix this for the next review :).

The other surprise for me is that its possible to modify the capacity of a
fixed_string. That means the class name fixed_string is misleading. Unless fixed
means 'not  broken' string implying that std::string is flawed. Its that sense
in which it  doesnt promise what it delivers. I would expect a fixed_string to
have a fixed length. This fits best with the rationale of fixed_string to be a
char array replacement. I suspect that is what potential users expected, but
nothing like what they got so far.  The solution now is to give users what they
expected in the first place.

>>Concern at the lack of detail in the
>>documentation and the way the documentation and the class itself are laid
>>out.
>
> I hope to improve the documentation so that users can follow it better.

Ok but there is also a lack of information. Class function signatures are
documented  but there is no information on what they do.  As a potential user I
get no sense that you as the author are keen about the class and are interested
in trying to help me to get to grips with it. It looks like the documentation
was sketched in and then just never worked on after that. The good thing though
is that you have a potential user-base of 400 to get this right for next time.
It would be worth going through the documentation of other (boost) libraries
that you like to see how the documentation is laid out. There are also some
examples of poor documentation in boost, so use those as examples of what to
avoid.

> I can only fix issues that are being raised.

Whatever... You need to figure why no issues are being raised. Answer I think is
that, because there is little information provided , then it is difficult to
raise any points about it by telepathy so to speak.

> That said, I intend to move the basic_string_impl class out of the detail
> namespace as this is more useful in other contexts. That class is based on
> the flex_string class written by Andrei Alexandrescu. However, this class -
> like the basic_string class - is *way* too large.
>
> I have experimented with splitting this class into several smaller ones
> based on the grouping in the standard documentation (iterators, capacity,
> etc.) so you can combine them. This way, it will be like the iterator
> classes. These have had some limited success and I intend on working on this
> after fixing the issues raised in the review.
>
> With this change, I intend to move the fixed_string class into boost/string
> so that other string classes like const_string can be placed there as well.

OK. Bear in mind that you have 400 potential users who wanted a fixed_string.
Its probably worth trying to find out what they thought a 'fixed_string' is  and
how it differs from what you provided. OTOH  It sounds now like you have opted
to design something completely different again, which seems a bit of a shame as
you will lose many of the potential fixed_string users.

regards
Andy Little







_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

John Maddock
In reply to this post by Hartmut Kaiser

> As you might have expected, I read through the reviews _very_
> carefully.
>
> There are two reasons, why I proposed to extend this review.
> 1) The library was downloaded by more than 400 interested people and
> I don't expect the 3 reviews to represent all of them.
> 2) Even if the currently reviewed library won't be accepted to Boost
> mainly for the reasons stated by the three reviews, I think we need a
> Fixed Strings library in Boost and the more dicussion we get the
> better for a future library.

As one as the silent downloaders, here's my non-review:

1) The introduction and rationale to the docs are very encouraging, they
lead me to think that there's a good little library here, but...
2) Once you get past the introduction things go downhill (sorry Reece).
Really the docs have all the problems that lead me to dislike Doxygen docs
in general: too much detail in all the places you don't need it
(implementation details) and too little detail where required: for example
the fixed_string class has next to no docs - just a list of constructors and
assign member functions.  Where are all the other basic_string interfaces?
The introduction seems to suggets that they should be there, but there's
nothing in the docs.  Could be that they're present in a base class, but as
a potential user *I don't want to know that*, I just want "give me the
interface, and give it to me fast".

Doc's asside let me throw in a couple of design questions, these are all
open issues BTW and just me thinking out loud really:

1) Should the fixed_string be an aggregate?

Currently we have Boost.Array that is an aggregate, but this design is not?
If it were fixed_strings could be statically initialised:

fixed_string<char, 10> s = { "abc" };

Potentially this leads to problems with the string length not being
initialised (or set wrongly), but I believe there may be solutions to that
(use the value zero as a "not set" value, and lazy initialise the length on
demand).

2) Do we need all the these policies?

There seems to be a lot of detail about various policies (formatting for
example) that I would have expected to "just work", while the one policy
that most people seem to want (overflow control) is missing).

Those were this issues that stood out to me, but with apologies to Reece, I
should stress that I haven't been able to give this the time it deserves.

Regards, John.



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

John Maddock
In reply to this post by Jim Douglas-2
> No self-respecting coding standard would allow you to write the code
> above. Rather it would insist that at least you wrote:
>
>     snprintf( buffer, 15, "Some %s text", "verly long" );
>
> Does that not solve the problem of overruns?

Um, which portable standard defines snprintf ?  :->

John.
_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

Reece Dunn
In reply to this post by Jim Douglas-2
Jim Douglas wrote:

>Reece Dunn wrote:
> > Normally, in C (or even some C++ code), you have constructs that look
>like
> > this:
> >
> >    char buffer[ 15 ];
> >    sprintf( buffer, "Some %s text", "verly long" );
>
>No self-respecting coding standard would allow you to write the code
>above. Rather it would insist that at least you wrote:
>
>      snprintf( buffer, 15, "Some %s text", "verly long" );
>
>Does that not solve the problem of overruns?

Coinsider:

   wsnprintf( buffer, sizeof(buffer), L"Some %s text", L"verly long" );

as the second example demonstrated. You are using the safe version of the
string API, but passing in an incorrect size due to an incorrect sizeof()
calculation. Where the above should be:

   wsnprintf( buffer, sizeof(buffer)/sizeof(buffer[0]), L"Some %s text",
L"verly long" );

- Reece


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

Bugzilla from markus.schoepflin@comsoft.de
In reply to this post by John Maddock
John Maddock wrote:

>> No self-respecting coding standard would allow you to write the code
>> above. Rather it would insist that at least you wrote:
>>
>>     snprintf( buffer, 15, "Some %s text", "verly long" );
>>
>> Does that not solve the problem of overruns?
>
> Um, which portable standard defines snprintf ?  :->

IEEE Std 1003.1, 2004 Edition

Markus

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

Reece Dunn
In reply to this post by Andy Little
Andy Little wrote:
>The other surprise for me is that its possible to modify the capacity of a
>fixed_string.

Are you sure? You can write algorithms that don't rely on a specific
capacity string. Thus, instead of:

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

you can write:

   int strlen( const fixed_string_base< char >& str )
   {
      return str.length();
   }

>That means the class name fixed_string is misleading. Unless fixed
>means 'not  broken' string implying that std::string is flawed.

The std::string interface has several flaws, such as it having too many
methods (see Herb Sutter's comments on this topic) and string types that
have different CharTraits not being compatible with each other. However,
fixed_string does not attempt to fix these. It is short for "fixed capacity
string".

>Its that sense
>in which it  doesnt promise what it delivers. I would expect a fixed_string
>to
>have a fixed length.

It has a fixed capacity, but variable length.

>This fits best with the rationale of fixed_string to be a
>char array replacement. I suspect that is what potential users expected,
>but
>nothing like what they got so far.  The solution now is to give users what
>they
>expected in the first place.

The above strlen example (and the rationale behind it) was that some users
(during the initial development) wanted to be able to write operations that
used fixed-capacity string such that they could redistribute the binaries
only. Thus the use of the fixed_string_base. An earlier reviewer has shown
me how to split these so you get the performance from using fixed_string and
the ability to operate on any fixed_string< n > object without templatizing
the method on n.

> >>Concern at the lack of detail in the
> >>documentation and the way the documentation and the class itself are
>laid
> >>out.
> >
> > I hope to improve the documentation so that users can follow it better.
>
>Ok but there is also a lack of information. Class function signatures are
>documented  but there is no information on what they do.  As a potential
>user I
>get no sense that you as the author are keen about the class and are
>interested
>in trying to help me to get to grips with it. It looks like the
>documentation
>was sketched in and then just never worked on after that. The good thing
>though
>is that you have a potential user-base of 400 to get this right for next
>time.

Noted.

>It would be worth going through the documentation of other (boost)
>libraries
>that you like to see how the documentation is laid out. There are also some
>examples of poor documentation in boost, so use those as examples of what
>to
>avoid.

The Spirit docs are very good :).

> > I can only fix issues that are being raised.
>
>Whatever... You need to figure why no issues are being raised. Answer I
>think is
>that, because there is little information provided , then it is difficult
>to
>raise any points about it by telepathy so to speak.

You are thinking of a circle ;). The reviews that there have been have
raised most of these points, so I will address these for the next review.

> > That said, I intend to move the basic_string_impl class out of the
>detail
> > namespace as this is more useful in other contexts. That class is based
>on
> > the flex_string class written by Andrei Alexandrescu. However, this
>class -
> > like the basic_string class - is *way* too large.
> >
> > I have experimented with splitting this class into several smaller ones
> > based on the grouping in the standard documentation (iterators,
>capacity,
> > etc.) so you can combine them. This way, it will be like the iterator
> > classes. These have had some limited success and I intend on working on
>this
> > after fixing the issues raised in the review.
> >
> > With this change, I intend to move the fixed_string class into
>boost/string
> > so that other string classes like const_string can be placed there as
>well.
>
>OK. Bear in mind that you have 400 potential users who wanted a
>fixed_string.
>Its probably worth trying to find out what they thought a 'fixed_string' is
>  and
>how it differs from what you provided. OTOH  It sounds now like you have
>opted
>to design something completely different again, which seems a bit of a
>shame as
>you will lose many of the potential fixed_string users.

I intend to keep the behaviour of the fixed_string class (modified,
according to the review feedback). This is what the unit tests are for :).
It is just *how* that is implemented that will change.

- Reece


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

Andy Little

"Reece Dunn" <[hidden email]> wrote
> Andy Little wrote:
>>The other surprise for me is that its possible to modify the capacity of a
>>fixed_string.
>
> Are you sure? You can write algorithms that don't rely on a specific
> capacity string.

OK I was under the impression you could (modify the capacity). Spell it out for
me in the documentation in this case.

> Thus, instead of:
>
>   template< int n >
>   int strlen( const fixed_string< n, char >& str )
>   {
>      return str.length();
>   }
>
> you can write:
>
>   int strlen( const fixed_string_base< char >& str )
>   {
>      return str.length();
>   }

Its possible to do this for fixed_string directly too.

#include <boost/fixed_string/fixed_string.hpp>
#include <boost/mpl/bool.hpp>
#include <boost/utility/enable_if.hpp>
#include <string>

namespace boost{

    template <typename T>
    struct is_fixed_string : mpl::false_{};

    template <int N, typename C, typename P1, typename P2>
    struct is_fixed_string<fixed_string<N,C,P1,P2> > : mpl::true_{};

    template <typename T>
    typename boost::enable_if<
        is_fixed_string<T>,
        int
    >::type
    strlen( const T& str )
    {
      return str.length();
    }
}

int main()
{
    // check doesnt interfere here
    const char arr[]= "hello";
    std::cout << strlen(arr) <<'\n';

    boost::fixed_string<20> str = "hello";
    std::cout << strlen(str);

    int n =9;
    //  std::cout << strlen(n); //error because not a fixed_string or char array
  }

>
>>That means the class name fixed_string is misleading. Unless fixed
>>means 'not  broken' string implying that std::string is flawed.
>
> The std::string interface has several flaws, such as it having too many
> methods (see Herb Sutter's comments on this topic) and string types that
> have different CharTraits not being compatible with each other. However,
> fixed_string does not attempt to fix these. It is short for "fixed capacity
> string".
>
>>Its that sense
>>in which it  doesnt promise what it delivers. I would expect a fixed_string
>>to
>>have a fixed length.
>
> It has a fixed capacity, but variable length.

OK.

[...]

> strlen example (and the rationale behind it) was that some users
> (during the initial development) wanted to be able to write operations that
> used fixed-capacity string such that they could redistribute the binaries
> only. Thus the use of the fixed_string_base. An earlier reviewer has shown
> me how to split these so you get the performance from using fixed_string and
> the ability to operate on any fixed_string< n > object without templatizing
> the method on n.

OK .

[...]

> The Spirit docs are very good :).

[...]

OK thats a good model . fixed_string should be easier to describe than Spirit
too.

> I intend to keep the behaviour of the fixed_string class (modified,
> according to the review feedback). This is what the unit tests are for :).
> It is just *how* that is implemented that will change.

What about the (lack of) overrun policy?

regards
Andy Little




_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

pabristow
In reply to this post by Hartmut Kaiser
| -----Original Message-----
| From: [hidden email]
| [mailto:[hidden email]] On Behalf Of Hartmut Kaiser
| Sent: 29 January 2006 23:31
| To: [hidden email]
| Subject: [boost] [Review] Fixed Strings review period extended

As another silent downloader, I just want to agree that we really do need a
fixed_string,
(a less than ideal name for what I would call a bounded_capacity string).
This one looks reasonable but I was unclear that it was the right one.

I suggest that the answer is to reject for now, but ask Reece to do a LOT
more work on the documentation.  It left me confused, and although this is
not an uncommon state ;-) it would appear that I am not alone.

Doxygen is NOT helping at all - it is deluding the author in thinking the
job is done automatically..

What we need here is a full discussion of the rationale - pros and cons -
for why this design is the least worst, at least.

(In the end, the language is at fault - it doesn't have built-in array-bound
checked arrays where the compiler at least knows the fixed maximum capacity.
While it might have been better if we hadn't started from there, we did, and
we are now seeking as good a bolt-on fix as possible.)

Paul

--
Paul A Bristow
Prizet Farmhouse, Kendal, Cumbria UK LA8 8AB
Phone and SMS text +44 1539 561830, Mobile and SMS text +44 7714 330204
mailto: [hidden email]  http://www.hetp.u-net.com/index.html
http://www.hetp.u-net.com/Paul%20A%20Bristow%20info.html


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

David Abrahams
"Paul A Bristow" <[hidden email]> writes:

> | -----Original Message-----
> | From: [hidden email]
> | [mailto:[hidden email]] On Behalf Of Hartmut Kaiser
> | Sent: 29 January 2006 23:31
> | To: [hidden email]
> | Subject: [boost] [Review] Fixed Strings review period extended
>
> As another silent downloader, I just want to agree that we really do need a
> fixed_string,

Why?

--
Dave Abrahams
Boost Consulting
www.boost-consulting.com

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

Gennadiy Rozental

"David Abrahams" <[hidden email]> wrote in message
news:[hidden email]...

> "Paul A Bristow" <[hidden email]> writes:
>
>> | -----Original Message-----
>> | From: [hidden email]
>> | [mailto:[hidden email]] On Behalf Of Hartmut Kaiser
>> | Sent: 29 January 2006 23:31
>> | To: [hidden email]
>> | Subject: [boost] [Review] Fixed Strings review period extended
>>
>> As another silent downloader, I just want to agree that we really do need
>> a
>> fixed_string,
>
> Why?

Usage of big stack buffer for string formatting was (is?) usual practice in
C programming. Especially for error message formatting. Nowadays need for
stack_string (which IMO would be a better name) is unclear. In practice it's
just an attempt on performance improvements. I guess it could be useful in
performance critical areas where need for dynamic allocation could hurt. An
alternative is having std::string somewhere cashed. But we a) may not place
for it b) still do not want to depend on performance on dynamic clear/resize
methods.

Gennadiy



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Review] Fixed Strings review period extended

Reece Dunn
In reply to this post by Andy Little
Andy Little wrote:
>"Reece Dunn" <[hidden email]> wrote
> > Are you sure? You can write algorithms that don't rely on a specific
> > capacity string.
>
>OK I was under the impression you could (modify the capacity). Spell it out
>for
>me in the documentation in this case.

Noted.

> > you can write:
> >
> >   int strlen( const fixed_string_base< char >& str )
> >   {
> >      return str.length();
> >   }
>
>Its possible to do this for fixed_string directly too.
>
>     template <typename T>
>     typename boost::enable_if<
>         is_fixed_string<T>,
>         int
>     >::type
>     strlen( const T& str )
>     {
>       return str.length();
>     }

But the solution is still using templates. You don't need to have a template
to use std::[w]string. Thus, if you wanted to move:

   __declspec(dllexport)
   void some_function( const std::string & str );

to use fixed_string with the implementation you mention, you would need to
expose the implementation of some_function to your customers and thus cannot
use it as part of a library (.lib) or DLL.

> > The Spirit docs are very good :).
>
>OK thats a good model . fixed_string should be easier to describe than
>Spirit
>too.

:)

> > I intend to keep the behaviour of the fixed_string class (modified,
> > according to the review feedback). This is what the unit tests are for
>:).
> > It is just *how* that is implemented that will change.
>
>What about the (lack of) overrun policy?

Coming soon to a fixed string library near you :).

The current unit tests will use the default overrun policy, which will be to
truncate the string if overrun occurs.

- Reece


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
123