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

From: Alexander Nasonov (alnsn_at_[hidden])
Date: 2006-01-20 07:39:55

I vote NOT to accept it for the following reasons:

> - What is your evaluation of the documentation?

The documentation says more about implementation details rather then public
interface.

There is only one informative sentence on fixed_string reference page:

"This class provides the implementation of an n-ary fixed capacity
string. It is an abstraction of a CharT[ n ] buffer that exposes a
std::basic_string-style interface."

All constructors and assignment operators don't have a documentation,
they're only listed. All other functions are not even listed!
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)

> - 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");
}

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.

I didn't try to compile anything because I simply can't compile anything:

$ cat test.cpp 

#include <boost/fixed_string/fixed_string.hpp>

int main()
{
}

$ pwd
/home/nasoale/src/boost/fixed_string
$ ls
boost  libs  tags  test.cpp
$ gcc -v
Reading specs from /usr/lib/gcc-lib/i486-suse-linux/3.2.2/specs
Configured with: ../configure --enable-threads=posix --prefix=/usr
--with-local-prefix=/usr/local --infodir=/usr/share/info
--mandir=/usr/share/man --libdir=/usr/lib
--enable-languages=c,c++,f77,objc,java,ada --enable-libgcj
--with-gxx-include-dir=/usr/include/g++ --with-slibdir=/lib
--with-system-zlib --enable-shared --enable-__cxa_atexit i486-suse-linux
Thread model: posix
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)

... skiped a lot of error lines

> - 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 :)))
I hope my negative review won't stop others from posting constructive comments.
--
Alexander Nasonov