Re: [boost] Review: Regular Expression Library

From: John Maddock (John_Maddock_at_[hidden])
Date: 2000-08-22 06:56:42

Jens,

Thanks for the comments:

> - In boost/detail/re_thrd.hpp, INFINITE is used although it is
>never defined.  Probably it should be
>std::numeric_limits<unsigned>::max()  from <limits> instead?
> - libs/regex/configure has an invalid check program around line
>2357, it should be pthread_mutex_destroy(&m) instead of ...(ps).

Yep, got it, just found those myself.

> - The libs/regex/old_include directory should go away for the
>final submission.

I'm in two minds about that one - that directory exists to allow existing
users to compile their code "as is" - without rewriting it to use the
updated "boostified" library.  I don't want to unnecessarily annoy existing
users (who have provided a lot of useful feedback in the past), or maintain
two differing version of the library, I guess if people are real keen on
this, then I could make that part optional (ie not part of the main boost
library).

> - Both the words "deprecated" and "depreciated" exist in my
>dictionary (with slightly different meaning), but the standard uses
>"deprecated" in Annex D, so we should probably follow the wording.

To be honest I think that depreciated is closer to the intended meaning,
but I'll check out the standard and think this one over....

> - The final submission should not include binaries for the demo
>programs, not even Linux ones :-)

I wasn't aware that there were any in the review zip (there shouldn't have
been).

> - I am missing some real introduction into regular expressions
>in the "introduction" section of the documentation: how do they
>look like, why are they useful, probably a few simple examples.

Yep, its on my list.

> - The doc says that bad_expression can be constructed from an unsigned
>int, the code does not provide this option.

Thanks, that was a late change that hasn't made it to the docs - well
spotted!

> - The documentation for reg_expression shows the default template
>arguments twice: once for the declaration and once for the
>definition. Strictly speaking, this is invalid C++.

True, but for documentation purposes, I think its a lot easier for the
default params to be in both places - it is documentation not code.

> - I suggest leaving out the deprecated members from the documentation.

For reg_expression?  Yes I think you are right.

> - regbase::flag_type is a bitmask type, but it does not define
>the appropriate operators | & etc.  regbase::flags() returns the
>wrong type.  For example, this code fails:
>
>        using boost::regbase;
>        regbase::flag_type f = regbase::use_except | regbase::newline_alt;
>
>Look at iostream's flags how to do it correctly.

Yep, I guess the alternative is to use "static const unsigned = value;"
syntax, but "that compiler" doesn't support that.

> - bad_pattern has an explicit constructor, but bad_expression doesn't.
>This is inconsistent.  I suggest makine all constructors explicit.

Yep.

> - reg_expression(const Allocator&) should be marked explicit.
> - same with reg_expression(const charT* p, ...)
>. - same with reg_expression(const std::basic_string<...> &p, ...)
> - same with reg_match(const Allocator& a)

Yep.

> - In general, care should be taken that a std::string is possible
>wherever a const charT* is allowed, for example in regex_format,
regex_merge.

Yep OK, I'll add some more signatures.

> - regex_split uses a container for output, but I think an OutputIterator
>would be sufficient and more flexible.

I take your point, but I've found a container much easier to use in
practice, anyone else have any thoughts on this?

> - regex.cpp:  bad_pattern::what() uses a local typedef for the base
>type, but bad_expression::what() does it directly.  This is inconsistent.
>I suggest removing the typedef from bad_pattern::what().

Nope, that's a VC6 workaround - cl can't cope with std::x::y::foo() syntax.

> - regex.cpp:  bad_pattern, bad_expression: Why do you implement the
>what() member function at all?  This will be delegated to the base
>class what() anyway.  If you need to implement some function so that
>the virtual table shows up in the DLL, use an empty destructor.

Yes that's the reason, does it matter which function is paced here? 
Whatever I'll change to the destructor if you prefer...

> - regex.cpp:  regbase constructors:  the copy constructor uses a
>member-initializer, but the default constructor doesn't.  This is
>inconsistent.  I suggest always using member initializers.

OK that'll cut the code down a bit  as well :-)

>I've got another point:
>
>I don't like the "sub_match" class.
>
>First, the documentation doesn't state what happens if
>matched == false and one of the conversion functions or length()
>is called.

Aha! I think you may actually have found a bug there - it should probably
return 0.

>Second, the class has many implicit conversion operators.
>All textbooks explain that one should be wary of implicit
>conversion operators.

Agreed.

>I may accept the conversion to std::string, since a sub-match
>is simply a (sub-) string.

Good.

>However, the conversions to int, unsigned int, short, unsigned short
>look strange.  As mentioned above, a sub-match is simply a string,
>so it is the duty of the caller to interpret whatever semantics
>he wishes into that string, including interpreting it as a number.

Yes, I think I'll take those out, trouble is, they're *extremely*
convenient at times, I wish their was some way to overload static_cast so
that explicit casts would be allowed but implicit casts not found.  Would
you be happy with a "int toi()" member function or is that tempting fate
<bg>?

- John.