Review: Regular Expression Library

From: Jesse Jones (jejones_at_[hidden])
Date: 2000-08-17 19:43:11

I don't have enough time to review all the code in detail, but I did look
over the high level interface and selected bits of the implementation and
I've found some minor issues:

* The high level interface RegEx includes a bunch of methods for grepping
through files. I'm not real keen on this: it makes the class less cohesive
and raises portability concerns because there are platforms like the Mac
where path names are not the preferred way of identifying files.

* RegEx has a ctor that will implicitly convert a const char* to a RegEx.
I'm not sure this is the right thing to do. If this was an explicit ctor it
would be more apparent that a lot of processing is happening under the hood
and clients would tend to think a little more about what they're doing.

* RegEx::Split returns an "unsigned". This looks a bit ugly since all the
other methods return an unsigned int.

* The RegEx docs are kind of skimpy on the results of OS errors (this
probably applies mostly to the file grepping).

* It's unfortunate that there's not a wide version of RegEx. Unicode is
getting more and more important and many apps only deal with Unicode
internally. I'd also like to see a bit of discussion on memory usage. For
example, if I use the low level stuff with wide chars will the code
allocate a bunch of 64K tables?

* Not all the exception code is wrapped with BOOST_RE_NO_EXCEPTIONS. For
example, sub_match::operator unsigned int in regex.hpp.

* In fileiter.cpp you'll find a method called mapfile::open. On the Windows
side CreateFile may fail. If this happens I think the error will be eaten
and the next file in the list will be processed. I'm inclined to think that
it would be better to throw if CreateFile fails. CreateFileMapping may also
fail. If this happens I think that the code will go off into the weeds
because _first and _last will differ.

* The Windows mapfile::close won't reset the member data if there was an
error opening the file. This might not be too bad, but open and close are
public methods...

* in cpp_regex_traits.cpp there's code that looks like this (eg in
cpp_regex_traits:: imbue):
   delete pmd;
   pmd = new re_detail::message_data<char>(locale_inst, regex_message_cat);
This is, of course, not exception safe. To be completely correct it should
assign the result of operator new to a temporary before deleting pmd. This
problem may well occur elsewhere in the code.

  -- Jesse