Re: [boost] [Review] Formal review of Fixed Strings library startstoday

From: Reece Dunn (msclrhd_at_[hidden])
Date: 2006-01-20 09:11:04

Alexander Nasonov wrote:
> > - What is your evaluation of the documentation?
>
>The documentation says more about implementation details rather then public
>interface.

Ok. I will try and address this when I rewrite the documentation.

>All constructors and assignment operators don't have a documentation,
>they're only listed. All other functions are not even listed!

I think this is because I am using the doxygen named group feature.

   /** @name Group */
   //@{
   //@}

and that is not pulling in all the functions :(.

>I realise that these functions are implemented in fixed_string_base
>but this is not documented (I'm pretty sure it's a typo, public keyword
>does never come alone)

I think this one is because the fixed_string_base (and fixed_string_impl) 
classes are in the details namespace and these are removed when the doxygen 
documentation is being pulled into the Boost docs.

> > - What is your evaluation of the design?
>
>fixed_string is derived from fixed_string_base. IIUC, the rationale is to
>get rid of compile-time N in fixed_string<N>. What is inappropriate,
>is that fixed_string copies arrays while fixed_string_base copies pointers.
>For example,
>
>fixed_string_base foo()
>{
>     return fixed_string<10>("Return a pointer to local array");
>}

I think Martin Adrian's solution would work well here. You would then have 
basic_string_impl (or something similar) to implement all the repetitive 
basic_string functions, fixed_string< CharT, n > for operating on 
fixed-length buffers and fixed_string_any< CharT > for operating on any 
length character buffers.

>Truncating the string silently is also not a good way. This often leads
>to errors at later stage of execution thus making it diffuct to track
>down an origin. For example, a developer appends ".ext" in one place
>to make sure that all files have extention, then in another place he
>rseaches for '.' expecting that it never returns npos.

Having an overrun_policy, as Martin suggests, would allow you to use a 
throwing policy to avoid this.

>I didn't try to compile anything because I simply can't compile anything:
>
>gcc version 3.2.2
>
>$ g++ -I. -I ../boost_1_33_0/ test.cpp
>In file included from ./boost/fixed_string/fixed_string.hpp:12,
>                  from test.cpp:1:
>./boost/fixed_string/detail/basic_string_impl.hpp: In member function
>`typename Base::const_iterator boost::detail::basic_string_impl<Base,
>ErrorPolicy>::begin() const':
>./boost/fixed_string/detail/basic_string_impl.hpp:149: error: there
>are no arguments to `begin_' that depend on a template parameter, so
>a declaration of `begin_' must be available
>./boost/fixed_string/detail/basic_string_impl.hpp:149: error: (if you
>use `-fpermissive', G++ will accept your code, but allowing the use of
>an undeclared name is deprecated)

In order to get this to work, I'll need to use the derived().begin_ 
technique used by the iterator implementations.

> > - How much effort did you put into your evaluation? A glance? A quick
> > reading? In-depth study?
>
>Quick reading.
>
>Sorry for all bad news for you, Reece. Es ist nicht so schlimm :)))

Was toten Unser nicht, macht Unser stärker. (Sorry if I have mangled the 
grammar, as I can't speak German fluently.)

Thanks for the feedback,
- Reece