[boost] Review: Regular Expression Library

From: John Maddock (John_Maddock_at_[hidden])
Date: 2000-08-19 06:34:23

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

I know :-)

This is a classic example of tight-coupling of the kind that Jeremy's been
trying to get us all to avoid.  However the fact is that users really seem
to like this interface, as one of my correspondents put it: "I can turn
interns loose on this stuff".

Longer term, I think that code should be rewritten in terms of Deitmar's
file iterators, so that all the platform dependent stuff is in one place, I
haven't found the time for that yet though, and if you want truely portable
then stick to the template regex code...

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

Hmmm, yes that should be qualified as explicit, same for the std::string
constructor.

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

Aren't "unsigned int" and "unsigned" the same thing?

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

Yep, I'll have to tighten that up.

>* 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?

Again longer term, I should probably add a wide character version, really
the RegEx wrapper class started out as just a demo, but people started
using it, so I thought I had better start documenting and supporting it.

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

I had thought that I had removed all references to  BOOST_RE_NO_EXCEPTIONS
(I'll double check): compilers that don't have EH support are no longer
supported.

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

Oops, serves me right for turning a demo into production code! :-)
Looks like I'll have to "toothcomb" that section again.

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

Got it.

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

Yep, got it.

>This problem may well occur elsewhere in the code.

Hope not :-)

Thanks for all the comments.

- John.