Re: [boost] string algo review (part 1)

From: Pavol Droba (droba_at_[hidden])
Date: 2003-10-22 08:13:07

Hi,

Thanks for your notes.

On Wed, Oct 22, 2003 at 12:54:29PM +0100, John Maddock wrote:
> The following is my initial impression of the string library based upon a
> review of the documentation, there is an awful lot here, so there is likely
> to be more to come (so no vote just yet).
> 
> 1) The introduction and usage sections look good: some initial thoughts:
> 
> a) The iterator_range class appears to offer a subset of the functionality
> of boost::half_open_range.  However I couldn't easily locate and
> documentation for half_open_range (Dave?), whatever we should combine these
> somehow.
> 

I didn't know that there is something like this in the boost. The closest thing,
I have seen is a range_view from sandbox. There is absolutely no documentation
for half_open_range.

Basicaly, I'm not against combining these two toghether, or to abandon the 
iterator_range in favor to half_open_range if it proves to be better.

> b) The split functionality appears to duplicate the functionality provided
> by boost::token_iterator and boost::regex_token_iterator - in general
> iterators provide a superior (pull) interface to that provided by split -
> that's why regex is deprecating regex_split in favour of
> regex_token_iterator.

string_algo library defines a concept of finder. This concept defines a way
how to factor out the "searching" from the algorithm. Split algorithms
in the lib allows to use the finders included in the library 
(or any other finder satifying the Finder concept) to do the splitting.

Internaly the split operations are implemented using custom iterator. Originaly,
I wanted to bring this as a user interface, but the type encoding was rather complicated.
Therefore I have abadoned this idea for a while. But I'm in favor to support
this iterator base interface. 

Maybe you can help me with some tips. 

> I agree with Beman that some of the support code should be brought forward
> for separate review - I'm less worried here about implementation details
> like the container traits - than I am about the public interface and the
> iterator_range/half_open_range issue.

As I stated before, I'm open for any ideas regarding the iterator_range. 
Such a facility is essential for the string_algo lib, but it is not yet
available elsewhere (or it is not documented). Once this issue will be solved,
and properly accepted in the boost, I would be more then willing the adapt the string_algo 
lib. 

I'll be glad to elaborate on this issue, if it is needed.

> 2) The reference section: we need an overview of the algorithms provided (I
> think someone suggested a table which looks to be a good idea), as it stands
> the user is faced with a long list of headers and not that much clue what's
> actually available.

It is currently very high on my TODO list.

> 3) Like others I'm not entirely sure that making the inplace versions of
> these algorithms the default is a good idea, personally I would prefer to
> see a name suffix, like to_upper_inplace or something (but lots of good
> suggestions have already been made on this issue).
> 
> 4) case_conv.hpp: Looks Ok at first sight.
> 
> 5) classification.hpp: The return type of these functions is an undocumented
> detail type (string_algo::detail::is_classifiedF), which should probably be
> a documented functor type IMO (compare with std::binder1st and bind1st for
> example).  It also wasn't clear to me what these functions did at first
> sight from the introductory blurb, and now that it is clear, I'm not sure
> why these functions are provided rather than a raw function object (compare
> with std::equal_to etc).

You have correctly identified a problematic part. During this review, I already
have some questions about it, and I already started to make some changes to it.

The idea behind them is that classification predicates are supposed to be combined 
using and/or/not operations. Beacause of some unfortunate reasons, I never actualy tried it
with something else then std::not1(). And so I haven't realised, that
stl, does not provide and/or combinators for predicates(which I don't know why
took as granted). 

Currently I have added and/or combinators to classification.hpp. Also the underlaying
functors will be redesigned so there will not be an need to specify "char" type.
Other changes include a compatibility with boost::lambda and droping the UnaryAdaptableFunction concept.

And BTW, the reason, why they are not implemented as raw functions is, that there
is a need to store some configuration parameters, like locales and a mathing set for is_any_of.

> 6) container_traits.hpp: needs a separate review: for example why both
> container_traits and container_value_type etc.  Why no container_ prefix to
> size, empty, begin etc.
> 
Originaly there was a monolitic, one class, interface for container_traits. 
There was a discussion on the list and the result is deprecation of one-class approarch 
in favor to freestanding metafunctions.

However, this stuff is quite hot now, and not fully shaked. I assume, that it will be
fixed in the near future. string_algo lib will be adapted then.

Regards,

Pavol