Re: [boost] string_algo review part 2

From: Pavol Droba (droba_at_[hidden])
Date: 2003-10-23 09:43:21

On Thu, Oct 23, 2003 at 12:12:11PM +0100, John Maddock wrote:
> Some more comments - still haven't got to the end - so still no conclusion
> yet:
> 
> 1) It would help if the banner at the top of each web page has either an
> "up" link, or a link to the string_algo index: I realise that these are at
> the bottom of the page, but it took me a while to find those :-)

Ask Doug, to change boost-book style, otherwise this will be standard for
many libraries in the boost quite soon. 

> 2) erase.hpp: I agree with Beman that your concepts need defining - mainly
> this is an issue of your template parameter names for me - what is an InputT
> for example?  Had it been called MutableSequence or something then we would
> have all understood you (and I think you do require a Sequence (21.1.1) not
> a Container (21.1) here right?).

There has already been discussion about this problem so I will not repeat it here. 
The point is taken and it will be changed. 

> 3) The copy versions of erase need better describing in the docs - if I've
> got these right (I haven't looked at the code yet) - then they don't
> actually erase anything - they just copy everything except the specified
> subset to a new destination.  I'm not even sure that erase is the right word
> for these, but I can't think of anything better, unless you recast them as
> copy_if or copy_not_if or something.

Rationale is that, they just variants of "erase" algorithms, so it would be
confusing if their names would differ from the inplace ones. 

> 4) I notice that the docs for individual functions don't mention the header
> containing the function: I'm looking at id5304717-bb.html in case that's the
> only one:-)

Same as 1.

> 5) find.hpp: Looks OK, but the comments about template parameter names not
> matching any known concept also apply.

Same as 2.

> 6) find2.hpp: What is the function find doing here - it doesn't fit with the
> stated intent of the header (to provide a set of generators) - and how does
> it differ from the algorithms in find.hpp?  Again I don't like the detail::
> return type - either document it, or make it implementation defined, but
> conforming to some concept.

The documentaion is generated form source codes using doxygen->boost-book
bridge. I don't know I to hide this definition from docs for now.
But I think, that this is an important case and I will have to be solved
somehow. I will post the query in the boost-doc list.

As for the find() algorithm. find_xxx are wrappers for  spefic finders.
They are only small forwarder, but they perform one important action.
Depending in the input argument, they select c::iterator of c::const_iterator
for the result.

This is important functionality and string_algo::find() provides this wrapping
to an arbitrary Finder object.

> 7) predicates.hpp and predicates2.hpp loom like they could/should be
> collapsed into one header IMO.

There is realy only a small distinction between predicated in these two headers.
The difference is, that the later ones allows a parametric element comparison.
Algorithms in predicate.hpp have this comparison fixed to is_equal() and is_iequal()

> 8) regex.hpp: InputT again, needs defining or replacing with the name of an
> already defined concept.

See 2.
 
> 9) In:   OutputIteratorT
>     replace_regex_copy(OutputIteratorT, const InputT &,
>                        const reg_expression< CharT, RegexTraitsT,
> RegexAllocatorT > &,
>                        const std::basic_string< CharT, FormatStringTraitsT,
> FormatStringAllocatorT > &,
>                        match_flag_type = match_default,
>                        match_flag_type = format_default);
> 
> Why have two flags parameter when one will do?  Regex uses the one, and IMO
> it's confusing to users to invent your own version here.... but I'm
> obviously biased.

Thanks for pointing this out. I really need your opinion about the integration
of regex into string_algo. 

This particular problem is there because in the previous version of regex,
there were two different flags for format and match. I have adopted the new
version only recently and obviosly I have missed this point.
 
> 10) find_all_regex: the ResultT is under defined, well actually not really
> defined at all.  I would have preferred an iterator in any case, likewise
> with the split_regex algo.

It is said that it should be 'container-container'. I understand that the more 
explicit specification is needed.

> 11) regex2.hpp: not much in here, so I don't see the need - can't we put
> this in regex.hpp?

See 12.

> 12) OK, I'm beginning to see the consistency in the *2.hpp headers now, but
> maybe if that's the intent, and you do want to keep them, then why not call
> them *_predicates.hpp, so we know what's what?  Just a thought.

*2.hpp headers contain generic variants of algorithms. There is a section about
the layered structure of the library in docs. *2.hpp represents the second layer.
(a.k.a string_algo namespace )

> 13) split.hpp: the ResultT needs defining again - particularly the type from
> which the ResultT::value_type will be constructed must be defined (I think I
> know what it will be from reading the docs, but it's not at all clear, lets
> have some guarantees as to what will work).
>

An example of valid ResultT is vector<string>. Important is that the ResultT::value_type
is a Container. There are some examples in the docs, how a special case, iterator_range
can be used as an inner type.

Requirement for outer and inner container is that is must be constructible from
a pair of iterators (i.e have vector<>( Begin, End ) like constructor)

I will put this to my list of issue, that has to be specified on the Concept level.

And as I said I would prefer iterator based variants of split. But I have one problem
with it.
I have implemented a "find_iterator". This class (you can have a look in string_algo/detail/find_iterator.hpp)
takes a Finder as an argument and then in each increment, use it to search for the next match.
Problem is that the signature of this class is rather complicated. It has 3 template
parameters, most of them templated again. So it is hard to define an variable with the required
type to hold the iterator (auto-type variable would be a miracle here). 
I was thinking about making a type-generator template, but it would not safe too much trouble, 
because you would have to specify all parameters anyway, only on a different place.

I haven't solved this problem so I rather droped this kind of interface for the split algorithms.

> 14) trim2 and trim.hpp don't have the relationship I suspected in #12 :-(
> 
See 12.

Regards,

Pavol