Re: [boost] Re: static sized strings

From: Rob Stewart (stewart_at_[hidden])
Date: 2004-05-25 14:52:10

From: "Reece Dunn" <msclrhd_at_[hidden]>
> Rob Stewart wrote:
> >
> >* I think that data() would be better named "buffer."  The
> 
> This would go with the renaming you suggest for functions that are the same 
> in the base and implementation classes (hiding issue).

data() is a const mf and returns CharT const *, but buffer() is a
non-const mf and can return CharT *.

> >* Should fixed_string support zero-length strings?
> 
> You can have zero-length strings in my implementation (this is what 
> fized_string() defaults to). What you *can't* have is a zero-*capacity* 
> string, which is the same as CharT[ 0 ].

I did mean zero-capacity.  I didn't look at your implementation
at this point, but I did notice that John's class declares a size
N array, so for N == 0, his would fail to compile.  Is the same
true of yours?

> >* fixed_string's at() shouldn't test the length and throw an
> >   exception since detail::basic_string_impl::at() does it and
> >   detail::basic_string_impl::operator[] calls it.
> 
> fixed_string tests against capacity as a measure against [] usage that would 
> cause buffer overflow, e.g.
>    fixed_srting< 5 > str;
>    str[ 10 ] = 'a'; // ok: exception
> vs
>    char str2[ 5 ];
>    str[ 10 ] = 'a'; // foobar

Fine, but there should only need to be one test per call to
at().  If size() can never exceed capacity, by enforcing
invariants elsewhere, then at() only needs to check that the
index doesn't exceed size().

> >* fixed_string::swap() is missing.
> >* detail::basic_string_impl::get_allocator() expects
> >   string_type::get_allocator() but that isn't listed in the
> >   requirements levied on the derived type and isn't defined in
> >   fixed_string.
> 
> These are currently missing becase (1) swap requires a deep copy and haven't 
> implemented that yet; (2) allocators are not used by fixed_string (no need 
> to allocate any data) so it is not implemented (I am considering if/how it 
> should be implemented). NOTE: it should compile despite these missing from 
> fixed_string unless you attempt to call those functions.

If you're not going to implement get_allocator(), then it should
be in basic_string_impl and shouldn't be among the requirements
levied upon derivates.

> >* I wouldn't expect fixed_string's StringPolicy to govern how to
> >   determine the length of a string passed to a mf as a CharT *.
> 
> It does not. string_type::length() returns the value of the string held in 
> the class (return a variable value, defer to container, use traits_type, 
> etc.). traits_type::length( const CharT * ) calculates the length of a 
> string passed to it, wher traits_type is a char_triats compatible interface.

>From your fixed_string.hpp:

   inline CharT * append( const CharT * s, size_type l = npos )
   {
      if( l == npos ) l = StringPolicy::length( s ) + 1;
      l = (( l + len ) > n ) ? ( n - len ) : l;
      CharT * ret = StringPolicy::copy( str + len - 1, s, l );
      len += l - 1;
      str[ len ] = CharT();
      return( ret );
   }

Notice the expression, "StringPolicy::length( s )?"  That's using
fixed_string's StringPolicy to compute the length of a CharT
const * argument.

> >* If I kept looking, I'm sure I'd find many more things, but you
> >   are more interested in philosophical differences, so I'll stop
> >   here.  Look at the end of the message for my summary.
> 
> I am actually interested in both. The feedback is appreciated and I will use 
> it to improve the code.

I'm starting to wonder if it would be good to reach some
consensus as to which approach is better, by some definition of
"better," so that the two of you can collaborate or one can stop
work.  I'm not certain we've reached that point, but it may be
wise to write a common set of tests that you both can run so that
performance can be compared (memory and speed) and binary size
can be assessed.  Given the adherence to std::basic_string's I/F,
both classes should work with the same tests, and usability comes
down to non-interface issues.

It would be useful to see the sorts of error messages generated
by the two approaches, too.  For example, if a derived class
fails to provide the right implementation function, how sensible
is the error message from the compiler?  If one generates much
more verbose and harder to decipher error messages, and nothing
else favors that one, then I would vote against that version.

> >I think John's approach is better, only because it does less type
> >indirection.  It relies on the more ordinary pvf approach to the
> >core functionality.  Otherwise, the two libraries provide the
> >same functionality (or can when complete).

> To simplify my design, I could implement basic_string_impl using the policy 
> approach that flex_string uses. The problem with this is that it requires a 
> usage like:
> 
>    flex_string< fixed_string< 10, char > > str;
> 
> so in order to get the fixed_string< n > syntax, I would need two classes 
> instead of just the one: this was something I was trying to move away from, 

Sorry, I don't know anything about flex_string, but what you've
shown would only serve to make your string class more painful
than John's (or your) current approach.

-- 
Rob Stewart                           stewart_at_[hidden]
Software Engineer                     http://www.sig.com
Susquehanna International Group, LLP  using std::disclaimer;