Re: [boost] Re: string_algo Formal Review

From: Pavol Droba (droba_at_[hidden])
Date: 2003-10-23 01:27:26

On Wed, Oct 22, 2003 at 08:41:42PM +0300, Peter Dimov wrote:
> Pavol Droba wrote:
> >
> > Problem is, that there is nothing like algorithms library currently
> > in the boost. Once, the other libraries will be reviewed, some
> > reordering can be performed.
> > It is hard to make a border between two countries, when there only
> > one.
> >
> > all() predicate was added to the string_algo library, because, there
> > was a request to handle classification of a string. (i.e a sequence
> > variant of std::isspace and etc). all() with a set of classification
> > predicated was a natural choice for this task.
> 
> I haven't studied the library in enough detail to write a proper review, but
> that's one of the issues I see with it, there is too much algorithm
> duplication:
> 
>     all(x, f) :- find_if(x.begin(), x.end(), not1(f)) == x.end()

Just to mention, that std::not1() requires the predicate to be unary_function. It 
is very strong requirement and disallows the use an ordinary function as a predicate
(not sure about lambda).

Anyway, I don't realy get why are you talking about some imaginary line.
There is no "all()" algorithm in the boost. This algorithm is related to the string processing,
a have already identified reasons why.
Thats is wrong about including it in this library. There are few easier things,
then movig it out to a more suitable plase, when the time comes and the other algorithm
libraries gets accepted in the boost.

However given the speed of the review process it can take a year or more.

> and it is not clear where the line has been drawn and why.
> 
> Part of the problem is that the library isn't really a string algorithm
> library, it's a container algorithm library with string-like operations.
> There isn't anything wrong with that but the focus is lost.
> 
> For example,
> 
>     bool isspace(std::basic_string<T> const & t, std::locale const & l);
> 
> is much less controversial than the generic 'all'.

Are you sure? Then you would need also

ispunct, islower, isdigit and etc. In addition to this you will need isspace_or_lower(), is_alpha_or_is_any_of()....

And what is so controversional about all()?

> 
> The other issue that I see after a cursory look can be summarized as
> 'trim_left_copy_if'. The verbosity of some of the names does not correspond
> to the frequency of their use. Once again, I'd rather see
> 
>     std::basic_string<T> ltrim(std::basic_string<T> const & input);
>     std::basic_string<T> ltrim(std::basic_string<T> const & input, Pr
> isspace);

trim_left_copy_if is not in the sources? where have you seen it?
Please try to have at least a brief look into docs, before making jugments like this.

Originaly, left trim function was name ltrim, but I was EXPLICITLY asked by a large number of people,
to rename it to more verbose, but also more explanatory format.

Last point is, why the string_algo supports more different containers. 
String processing is not only a matter of std::string. There are other essential structures, for instance C-string.
It would be possible to constraint the library to std::string, but what benefit it would bring over the current design?

Imagine a function:

replace_first( Input, Substring, Replacement );

Under the current design, you can use any container for each of parameter. It can be even char*, or char[]. It can be
a unicode a vector or anything else satisfying the requirements.

You are suggesting this

replace_first( const std::basic_string<>& Input, std::basic_string<>& Substring, std::basic_string<>& Replacement );

Eigher you povide additional oveloads for at least char* and wchar*, it this interace is very limited.

I think, that in the current state, having the library to support only one container would be a giant step back.

Regards,

Pavol