Re: [boost] [Review] Fixed Strings review period extended

From: Reece Dunn (msclrhd_at_[hidden])
Date: 2006-01-30 06:56:09

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