Re: [boost] Re: string algorithms review

From: Pavol Droba (droba_at_[hidden])
Date: 2003-10-18 07:46:51

Hi,

Thanks for your comments.

On Sat, Oct 18, 2003 at 01:36:28PM +0200, Daniel Frey wrote:
> I looked at the library and I think it should be accepted. I have one 
> big concern, though, which IMHO must be addressed:
> 
> 
> * The default should be copying algorithms, not modifying algorithms!
> 
> When I write to_upper( str ), a copy of str should be returned. This is 
> important as most users IMHO expect this. Maybe some other syntax for 
> the modifying version can be found. Some ideas:
> 
>   to_upper_inplace( str ); // easy
> 
> or
> 
>   apply( to_upper, str );
> 
> The latter is possible if to_upper is an object of a type to_upper_t 
> with appropriate operators:
> 
>   std::string to_upper_t::operator()( const std::string& str ) const;
>   std::string& apply( const to_upper_t&, std::string& str );
> 
> I thinks it's reasonable to expect those that want optimizations to do a 
> little more work and those that don't care and want the easiest syntax 
> to have a safe library.
> 

In the beginning of the library implementation, the copy versions were default.
However, than there was a discussion on the list and people demanded, that
the default should be inplace.

I don't remember the exact reasons now, but one of them was the similarity
to the STL. Algorithms there are using the same convention (i.e. copy variant has suffix).

I don't have any strong preference for either of conventions. But naming is very
subjective matter, so I would like to hear more votes for the change before I'll
go and do something.

As for the "apply". This is not very reasonable, since in the majority algorithms,
the implementation of copy and inplace version are quite different.

> 
> * Another point I was missing is the classic counterpart of trim: pad. I 
> think that this algorithms should be added to extend a string to a given 
> minimum length. I think most people know what pad (pad_left, pad_right, 
> ...) does, if you want me to elaborate, just let me know.
> 

This is a good idea and it was already proposed. I will incorporoate this
facility.

> 
> * I missed a small overview table to show which algorithms are provided, 
> probably at the very beginning of the library. The closest thing I could 
> find is the header overview, but I'd like to have a much briefer table 
> in the first section of the documentation.
> 

It wasn't added, because there is quite a lot algorithms there, so it would
not be very brief. But it seems that it is required so I will add it.

> 
> * The documentation contains the docs for detail::-stuff. But, well, 
> those are details and probably are only bloat for the documentation. 
> Even worse: People will use them, as they are documented :)
> 

The library is layered. I assume, that you are refering to the second layer 
(i.e. namespace boost::string_algo). This is not a detail::-stuff. It it 
designed to be used by advanced users. I think, that it could be very usable,
and I would encourage the users to try it and use it.

> 
> * Some typos: grep for "mathing" (should probably be "matching").
> 
Typos, well, I'm not a native english speaker. I would appreciate any help to cleanup
the docs.

Regards,

Pavol