Re: [boost] string_algo Formal Review

From: Pavol Droba (droba_at_[hidden])
Date: 2003-10-20 11:28:08

Hi,

Thanks for you opinions. Please, let me explain.

On Mon, Oct 20, 2003 at 09:05:18AM -0400, Beman Dawes wrote:
> Although I certainly think the library fills a very real need for 
> additional string algorithms, there are serious problems with the library 
> as proposed. Since the problems prevent a complete review, I vote that this 
> library not be accepted.
> 
> Here are the serious problems that prevent a complete review:
> 
> Two libraries intertwined
> -------------------------
> 
> There really two libraries here:
> 
> -- Some additional (and welcome) low level algorithms, particularly useful 
> with strings.
> 
> -- A framework of Container Traits, Sequence Traits, and iterator_ranges 
> which can be used to construct algorithmic functions interfaces and 
> implementation at a slightly higher level than traditional begin, end, 
> iterator arguments.
> 
> Mixing the two together in one review causes me great concern. I'd really 
> like to evaluate the framework aspect by seeing it applied to the familiar 
> Standard Library STL algorithms. If the framework isn't beneficial for 
> those existing algorithms, I'm really not interested in it for strings 
> alone. Yet all the framework uses in the library deal with the new string 
> algorithms.
> 
> Likewise, I'd like to review the string algorithms separately. As it stands 
> now, when I try to review one of the string algorithms, I get lost in 
> issues relating to the framework and can't give the algorithm itself full 
> attention.

I agree that the sequence_traits and container_traits should be separated. However
they are not currently in in the boost. In the documentation there is explicitly
stated, that, for instance, when container_traits will be accepted to the boost,
string_algo library will use those.

There are many libs in the boost, which contain subparts, suitable for separation,
yet there is no problem there.

IMHO container_traits brings a lot of additional value to the string_algo library,
sequence_traits are even more essential.

Unfortunately, it is not possible to leave these two componetents undocumented, because
they have an essential impact on the library design.

iterator_range is merely an utility to make some tasks of in the library easier. I don't
know why do you pay so much attention to it. If such an utility will be contained
in a separated (or more specific) library, I'll be glad to remove it from the string_algo library.

You can also look at the inclusion of container_traits and sequence_traits as a side effect of
the review of the string_algo lib. When the time comes, to review the separated versions of
these compenents, there will already be a "working practice" so the review will be easier.
Just a example for this approach, try to remembering enable_if.
 

> Failure to specify foundation algorithms
> ----------------------------------------
> 
> The foundation of any STL-like algorithm extension library are the 
> functions which operate on iterators and iterator ranges. If these are 
> deficient, convenience wrappers only paper-over the problems.
> 
> Yet in the string_algo library, the foundation algorithms are placed in 
> namespace detail and are not documented. Thus it isn't possible to 
> seriously review them. (Reading the code doesn't count, as far as I'm 
> concerned.)

Don't you think, that there is a time to look beyon the STL? Look the list
for so-much wanted container_traits and now being-developed container_algo(s).
I personaly think, that the STL interface is obsolete and should be replaced
with something more simplier to use. Therefor the string_algo library abandons
the STL like interface completely in the favor of new, container-oriented one.

But I don't understand your point about the implementaion of algorithms being in the 
detail namespace. What's wrong with it, and what additional documentation do you request?

Even in the STL specification, the definitions of the algorithms
are quite vaque in the terms of the implementation. There is no requirement
for the user to know how the particular algorithm is implemented. I have followed
this approach, because I find it reasoable. This library is in the first version,
and I don't think, that the optimization and algorithms performance is the most
important issue right now. 

Important is the stable interface. I will do my best in the future, with the help
of community, to optimize internals of the lib, if it will be necessary.

When I started to develop this library, I was driven by the need for a set of simple
algorithms which are needed every day, like trim and etc. As far as I know, there
is a lot of people, who will be glad to have something like this, and they would not mind,
that the algorithms will be suboptimal in the first iteration.

> Underspecification
> ------------------
> 
> The documentation is so short on specifics that it is not possible to 
> evaluate the library. For example, the "trim" function is specified thus:
> 
>    Synopsis
> 
>    template<typename ContainerT>
>      ContainerT &
>      trim(ContainerT & Input, const std::locale & Loc = std::locale());
> 
>    Description
> 
>    Parameters
> 
>    Input
>      An input container
> 
>    Loc
>      A locale used for 'space' classification
> 
>    Remove all leading and trailing spaces from the input. The input
>    sequence is modified in-place.
> 
>    Returns: A reference to the modified input
> 
> The lack of any further specification (such as requirements or concepts) 
> for ContainerT prevents understanding the limitations of the function. This 
> same lack of specification for template parameters is pervasive throughout 
> the library. Likewise, the documentation pervasively fails to specify 
> preconditions, exceptions which might be thrown, or postconditions. 
> Specification of effects or returns, where present, tends to be informal 
> and underspecified. These documentation failures prevent a reviewer from 
> seriously reviewing the library.

The need for proper concept checking, and specification has already being arised.
I have it on my todo list. Problem is that I need to refince STL concepts, because
they lack the needed granularity. I was looking into Boost.Concept library, but it
is not complete enough. 

And, taking your ideas, additional concepts which are not directly related to the library,
would for, just another component like container_traits. So I would rather propose the
update in the Boost.ConceptCheck library, followed by the update of the string_algo lib.

Regarding the exact specification. There are libraries in the boost which favor user-friendly
documentation to rigorous TR like specification. These specification can be added to docs later,
but I don't understand, why do you think, that without them, the library is unusable?

And BTW, the library does not throw any exception on its own.

> For the iterator_range packaging of two iterators into an object which 
> provides a container interface, it is particularly important to know the 
> concepts/requirements on the iterators, because these flow through to the 
> resulting container. Without any specification or documentation about how 
> the category of iterator affects the resulting pseudo container, it is hard 
> to know if this is a worthwhile approach or not.

Well, the iterator_range is a minimalistic wrapper. It merely wraps the two iterators,
iterator_range, provides exactly the same power as the contained iterator.
As stated in the docs, it is a replacement for std::pair<it,it>, what additional
information would you like to see?

I hope, that my explanation partly answers your questions.

Regards,

Pavol.