[boost] Re: shifted_ptr review

From: Fernando Cacciola (fernando_cacciola_at_[hidden])
Date: 2003-09-30 12:58:23

This is my review of the shifted_ptr library.

As of this moment I vote to reject the library based on current problems in the following areas:

Documentation:
~~~~~~~~~~~~~~
As others already mentioned, right now it is largely inadequate.
Most of it merely repeats textually what the code 'says itself'.
It is simply wastefull to repeat textually what is clearly expressed by the code itself.
Documentation is intended to explain what is not obvious from the code, the rationale, how to
use it, etc...

Coding style:
~~~~~~~~~~~~~
This doesn't look like typical boost code, for at least these two reasons:
Headers are full of tabs.
Comments take up unnecesarily most of the header space

offset.hpp
~~~~~~~~~~
I wouldn't use a distinct type emulating size_t (that is, type "offset")
just to be able to perform pointer arithmetic using builtin operators.
I rather use std::size_t directly with custom "functions" to do
the desired arithmetic. In any event, I'd certainly call it "offset_t"
The function "boost::pos" is way too short-named, specially for a function
living at namespace boost and taking a generic member pointer.
Furthermore, this function uses "reinterpret_cast".
Uses of this cast should give a rationale explaining what are the
assumptions about the expected behaviour, and, if possible,
should give a detailed list of compilers comforming to that behaviour.
Still, this implementation seems to duplicate the "offsetof" macro
which is available on most compilers, so I'm not sure if it is
really needed.
The overloaded functions "boost::len" are way too short-named and I don't
understand why are they needed at all since they merely wrap a simple sizeof()
expression

shifted_ptr.hpp
~~~~~~~~~~~~~~~
shifted_impl<> is not correctly implemented as a singleton: it is a struct, with
all public methods and nothing to prevent users from instantiating more than one.
Its design is incorrect: it has a 'clear()' method which deallocates all registered
memory blocks via std::free, yet it doesn't set any explicit restriction nor does
it document how those blocks must be allocated in the first place.
If this is intended as an utility class used only by the shifted_ptr<> implementation
it should be documented as such and properly hidden (moved to another header)
The mechanics are too intrincate and I wouldn't dare to use this without a clear
understanding of how it works. The documentation should include at least a brief
description of such mechanics.

General:
~~~~~~~~
Many size_t's along the headers appear unqualified. size_t is a standard name,
not a built-in type, so it must be spelled std::size_t unless a using
declaration/directive brings it into context.

Fernando Cacciola