Re: [boost] Review: Regular Expression Library

From: Jens Maurer (Jens.Maurer_at_[hidden])
Date: 2000-08-20 16:00:08

Here are my review comments as a boost member.  Please don't
confuse this with my role as review manager.

Compilation
-----------

I used "configure" with gcc 2.95.2 on Linux/x86 to compile the library
with thread support.

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

Packaging
---------

 - The libs/regex/old_include directory should go away for the
final submission.
 - 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.
 - The final submission should not include binaries for the demo
programs, not even Linux ones :-)

Documentation
-------------

 - 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.
 - The doc says that bad_expression can be constructed from an unsigned
int, the code does not provide this option.
 - 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++.
 - I suggest leaving out the deprecated members from the documentation.

Interface
---------

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

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

 - 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)
 - 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.
 - regex_split uses a container for output, but I think an OutputIterator
would be sufficient and more flexible.

Implementation
--------------

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

Jens Maurer