[boost] string_algo review part 2

From: John Maddock (john_at_[hidden])
Date: 2003-10-23 06:12:11

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 :-)

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?).

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.

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:-)

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

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.

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

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

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.

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.

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

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.

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).

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

I'll start testing next,

John.