Re: [boost] [xpressive] formal review

From: Eric Niebler (eric_at_[hidden])
Date: 2005-09-24 10:36:17

Thanks for taking the time, John. Some comments inline.

John Maddock wrote:
> Finally, too late for even the extended deadline, here's my xpressive 
> review:

Actually, the deadline was extended to tomorrow, so you're good.

> *What is your evaluation of the design?
> 
> It's very good.
> 
> I have to admit that I find the static regexes very hard to read, but a lot 
> of that will be a cultural problem: I'm so used to seeing normal regex 
> syntax that I can't quite get my head around the static versions, the 
> quantifiers as prefixes particularly throw me. However, the syntax is 
> concise and consistent with spirit, so I can hardly complain that much. 
> Possibly xpressive isn't the last word on this, or the best way to go, 
> personally I prefer more verbose forms like
> 
> repeat(x)

There is a repeat function for feature-parity with {n,m} style 
quantifiers, but it could also be used in place of * and + if you 
prefer. For instance this:

   *x

is equivalent to:

   repeat<0,inf>(x)

> but again, that's an initial reaction which may change with more 
> familiarity.

Quite possibly.

> * What is your evaluation of the implementation?
> 
> Looks good to me, and I know from past experience that Eric knows what he's 
> doing.
> 
> The only complaint I have is one that's common to all expression template 
> code: it's impossible to understand by debugging, which means casual 
> maintenance by folks other than the author would be hard, you really need to 
> read the whole source before you can get a grip on what it's doing (OK you 
> ought to do that anyway, but it is still hard to understand code that isn't 
> executed in the traditional sense).  As I said though this is an issue with 
> all metaprogramming, and expression templates in particular, I guess what we 
> really need is a compile-process debugger :-)

Indeed. I wonder if you were debugging the execution of a regex, or the 
compilation of one. The code that gets executed during regex matching is 
actually pretty straight-forward. Most of what you need to know is in 
xpressive/detail/matchers.hpp, which should read fairly well.

The expression template code is much harder to read (and write!). I 
tried to factor much of it out into a reusable sub-library 
(xpressive/detail/proto/). It generates executable code from an 
expression template given a set of MPL-like transformations. If this 
tool got wider adoption, perhaps it would give us a common language with 
which to talk about this stuff.

You're right though, a compile-process debugger would rock.

> 
> * What is your evaluation of the documentation?
> 
> It's excellent.
> 
> * What is your evaluation of the potential usefulness of the library?
> 
> Clearly there are lots of uses.  There is also overlap with Regex and 
> Spirit, but I don't believe that should prevent acceptance, Boost is meant 
> to drive forward innovation after all.
> 
> * Did you try to use the library?  With what compiler?  Did you have any 
> problems?
> 
> Not for as long as I would have liked :-(
> 
> I've only tried with VC7.1, the sample code compiled cleanly, but compile 
> times were quite long for even simple static regexes, I didn't try anything 
> complex.
> 
> * How much effort did you put into your evaluation? A glance? A quick 
> reading? In-depth study?
> 
> A couple hours reading the docs, and playing with some of the samples.
> 
> One thing I did notice is that several of the test programs don't appear to 
> do anything except "return 0", I'm not sure if these are deprecated, or just 
> not finished (see test_regex_token_iterator.cpp for example).

There are a bunch of tests that simply ensure that each of the public 
headers compiles cleanly on its own. I should change these from "run" to 
"compile" in the test matrix and add some comments in the cpp files.

> The test cases in the testX.cpp files may be a little short on the ground as 
> well: Boost.Regex has about a thousand cases, PCRE a similar number, where 
> as there are 171 here.  The smaller number of cases ought to be enough, but 
> in practice I've found you can never have too many!  It's a pity that the 
> Boost.Regex and / or PCRE cases can't be automatically harvested, but I 
> suspect this is next to impossible for static regexes?

Not impossible, or even that hard really. It has long been my intention 
to write a tool that takes a perl-style regex and spits out a static 
xpressive style regex. (Naturally, it will be written with xpressive!) 
It has also been my plan to try to port as many of the Boost.Regex test 
cases to xpressive as possible.

Of course, compiling 1000 static regexes could take a looong time, so I 
might need to be a bit more targeted in my approach.

> 
> * Are you knowledgeable about the problem domain?
> 
> I hope so :-)

Indeed, you are! :-)

> 
> * Do you think the library should be accepted as a Boost library?
> 
> Absolutely.
> 
> John. 
> 

Many thanks.

-- 
Eric Niebler
Boost Consulting
www.boost-consulting.com