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

From: Joel de Guzman (djowel_at_[hidden])
Date: 2002-10-11 19:38:48

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

Hi,

First of all, thanks!

> What would the namespace and the macro prefix be for Spirit
> in Boost? boost::spirit and BOOST_SPIRIT? Another Boost
> convention is to use `detail' for implementation directories
> and namespaces. Spirit currently uses `impl'. Would this be
> changed?

Iff spirit gets accepted, namespace and the macro prefix will
boost::spirit and BOOST_SPIRIT. However, since detail
is an implementation convention, I'd prefer to keep impl if
that will not (as I assume) affect the review, as a matter of
choice.

> Also, if Phoenix is not currently under review, would the
> Phoenix dependancies be removed from Spirit when it is merged
> with Boost? Will Phoenix ever be submitted for review?

Boost.BLL will be fully supported. There is no dependency on
Phoenix apart from some of the examples. The library itself does
not depend on Phoenix.

> Section headings such as `chlit [ ch_p ]' on the Primitives
> page are confusing, especially given Spirit's various uses for
> operator[]. `chlit and ch_p' would be clearer.

Understood.

> The `f' prefix for the names functional components is not
> very readable to me. Maybe a namespace would look nicer, with
> `functional::chseq' instead of `fchseq'. Maybe just using more
> underscores would be good enough: `f_ch_seq'.

Ok. 

> The C grammar example program doesn't look like it supports
> string literals containing escaped double-quote characters
> ("\""). This looks like a typo, `strlit<>("\"\"")' instead of
> `strlit<>("\\\"")'.

I CCd this to Hartmut Kaiser. He's the author of the C parser
demo.

> When writing a grammar, Spirit's pseudo-BNF syntax makes it
> easy to make the following mistake:
> 
>   rule<> a, b, y, z;
>   a = y | z ;
>   b = y ;         // oops, y isn't assigned yet
>   // ... assign y and z
> 
> I see now that either of
> 
>   b = y | nothing_p;
>   b = y.alias();
>   
> will probably do what was intended. It seems there is an unfortunate
> syntax collision. It'd be nice to have operator= defined such that the
> above example works as expected. Maybe an `assign' member function for
> rule could do what operator= currently does. Maybe also then `alias'
> wouldn't be needed anymore.

Pre 1.5 Spirit did it that way. Yet, here's the rub: doing so
would prevent rules from being storable in containers. Previous
experiments with dynamic parsers necessitated rules that are
copyable and storable in stacks, vectors etc. 

> There is a spirit::iter_policy_t, and in a few places in the
> documentation there is code that defines a new typedef named
> iter_policy_t, such as in scanner.html .
> 
> Instances of rule have hard-coded scanner types. This is inconvenient,
> and the documentation suggests that this is unavoidable. Naively,
> however, it seems that rule doesn't really need to know exactly which
> scanner it will be used with, but instead only certain traits of that
> scanner. If there's some reason why rule really has to have a scanner
> template parameter that the authors already know about, it would make
> a great entry for the rationale page. Otherwise, I'd like to look into
> making rule more flexible.

This is a consequence of the dynamic nature of rules. Rules
straddle the border between static and dynamic C++.
It might not be obvious but the rule is really tied to the type
of the scanner. Why? Because virtual member functions cannot
be templated and the rule uses runtime-polymorphic classes
internally.

This unfortunate inconvenience can be overcome:

1) When C++ adopts the "auto" keyword as suggested by David Abrahams.
2) By using subrules.
3) By using grammars.

Experience shows that it is best not to use *bare* rules, but instead
wrap them in grammars. The grammar will take care of the scanner
mapping business.

> Err, it would make a great start for a rationale page, as Spirit doesn't
> have one yet :-).

Agreed.

> Types like anychar_ look like they should be moved into an
> implementation-detail namespace.

Why?

> How does one instantiate the four-parameter ast_parse function template?
> I don't know if I just haven't done it right or if there's a bug in
> Spirit. It isn't ever done in the examples, and there doesn't appear to
> be tests for any of the tree components :-(.

I CC'd Dan Nuffer. He's the author of the AST/Parse trees.

Again, thank you very much.
--Joel