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

From: Dan Gohman (dgohman_at_[hidden])
Date: 2002-10-11 18:33:47

I haven't finished reviewing the library, but here are my
initial comments, based on some quick attempts to use it
for some small trivial programs as well as a medium-sized
grammar.

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?

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?

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.

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

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<>("\\\"")'.

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.

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.

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

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

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 :-(.

Dan

-- 
Dan Gohman
dgohman_at_[hidden]