Re: [boost] Formal Review: The Spirit Parser Framework.

From: Joel de Guzman (djowel_at_[hidden])
Date: 2002-10-19 19:35:43

----- Original Message ----- 
From: "Dan Gohman" <dgohman_at_[hidden]>

> Here is my review of the Spirit library. I think Spirit should be
> accepted.

Thank you very much.

> I have several comments, many of which could be addressed with better
> documentation. The documentation that is there is mostly good, but
> there should be more.

Based on people's comments, I agree completely. I really
appreciate these comments. These are certainly going to 
improve the documentation a lot.

> In yacc, a common idiom is to use semantic actions like
> { $$ = new_foo_node($1, $3); } to build up a tree structure with my own
> types, and it isn't clear how to do something like this with Spirit.
> Short of having a page dedicated to porting from yacc, this specific
> issue should be addressed.

Understood. I realize the importance of this. 

> I looked at Spirit's AST support, but for my purpose I would need to
> translate Spirit's AST into an AST using my own types. It would be better
> if I could construct this tree directly. Also, finding a typo in
> ast_parse that prevented it from compiling as well as discovering that
> none of the tree components have tests made me a bit shy.

I will definitely spend more time on the trees as well as the
handling of semantic actions. Both of these are very important.
After all this is the actual output of the parser. 

Actually, if I haven't mentioned yet, John Bandella (the boost
tokenizer author), contributed another tree sub-framework
last month. Unfortunately, I wasn't able to integrate his code
and documentation in time for the review.

> Closures seemed to be more complicated that what I needed; the large
> section in the documentation discussing closures as mechanisms for
> accessing variables from an enclosing scope was distracting and made me
> look for a simpler solution. Also, it wasn't clear how to control the
> result value, the '$$' in yacc terms, of a parse using closures.

"The closure member1 is the closure's return value. This return value, like 
the one returned by real_p, for example, can be used to propagate data 
up the parser hierarchy or passed to semantic actions."

I will attempt to restructure the section on closures to make it less
distracting. I think the YACC like groups will make closures
and attribute handling significantly more natural.

> The brief mention of contexts in the documentation made them seem
> promising, and while the documentation mentions that context is part of
> the public API, it isn't documented. I dug into the source to figure
> out what was needed and wrote my own context class. The result is code
> that compiles but fails at runtime while trying to access an
> uninitialized reference. I haven't determined where the problem is yet,
> though.

Any specific reason for the interest in parser contexts? Actually,
its purpose is to make the framework extensible. 

> An inconvenience with Spirit is compile time, especially for larger
> grammars. Bison certainly has an edge here ;-). If the Spirit authors
> have techniques for reducing compile times, they should mention them in
> the documentation. For example, some of my grammars have very large rule
> definitions. Is it effective to break rules up into component rules?

Indeed. Right now, no real effort yet has been invested towards this goal.
As you all understand, the original goal of Spirit was for small to moderate
grammars. I was amazed to see projects such as JCAB's C++ parser
pushing through. We are really pushing the compiler hard. Yet, I'm sure
there are lots of potential for improvement in this area. 

> Also, a documentation section for common mistakes would be useful. Often
> the messages coming out of the compiler are difficult to interpret.

Ok. Noted.

> The grammar class template works well enough to separate rules from
> specific grammars, however the grammar documentation should make it
> more clear why a grammar is useful. The part the begins ``Decoupling
> the scanner type...'' contains motivating information which should
> be in the very first paragraph.

I see. I'm looking at it right now.

> I've had a breif perusal through the code, and I generally like what I
> see, although I'd need more time to look through it more carefully
> before making any meaningful conclusions.
> 
> Why does the action class template have a default constructor?

IIRC, this is due to a problem with either Borland or MSVC
and propagated outside the original Spirit-X experiments. I
will investigate this further. 

> I've now seen some of the discussion about the group parser development,
> and this seems interesting. I'll have to look though the archives and
> learn more about it.

This will surely be the main focus in the coming weeks. In general,
there is a planned "meta" module that will be a set of meta-functions, 
specifically for meta-parser transformations.

This module, I believe will be the key towards a common need such as
refactoring parsers and the group parsers, etc.

> Thanks again for a very useful library.

Most welcome, and thanks a lot for reviewing the framework.
I'll do my best to address your comments and suggestions.

Regards,
--Joel