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

From: Joel de Guzman (djowel_at_[hidden])
Date: 2002-10-14 23:49:00

----- Original Message ----- 
From: "Carl Daniel" <cpdaniel_at_[hidden]>

Hi,

First of all, thank you very much for micro-scrutinizing the
code. I appreciate this a lot.

> More nits - scanning through headers.
> 
> -cd
> 
> #if defined(BOOST_MSVC) && (BOOST_MSVC <= 1300)
> #define SPIRIT_TYPENAME
> #else
> #define SPIRIT_TYPENAME typename
> #endif
> 
> Appears in a number of files - perhaps it should be replaced with a common
> mechanism rather than repeating it in each & every file where it's used.

At the end of the file, there's an #undef. This localizes the 
macro to the specific file rather than make it global. It would be
trivial to do as you suggested though. Thoughts?

> ---------------------
> 
> /boost/spirit/debug.hpp:
> 
> // by the user. If SPIRIT_ASSERT_EXCEPTION is thrown, then that will
> 
> should read
> 
> // by the user. If SPIRIT_ASSERT_EXCEPTION is defined, then that will

Ok.

> ---------------------
> 
> /boost/spirit/attribute/closure.hpp:
> 
> BOOST_STATIC_ASSERT(SPIRIT_CLOSURE_LIMIT <= PHOENIX_LIMIT);
> 
> If Spirit doesn't depend on Phoenix, why is this here?  (It appears that
> this part of Spirit does in fact depend on Phoenix - is this part of what's
> being reviewed?).

I'll answer this in a separate post.

> ---------------------
> 
> /boost/spirit/core/actions.hpp
> 
> - contains mixture of tabs & spaces for indentation

Ah, thanks for spotting this.

> --------------------
> 
> /boost/spirit/core/composite.hpp
> 
> classes unary<S,B> and binary<S,T,B> require that S::embed_t (or X::embed_t,
> Y::embed_t) be defined as a type, but this requirement is not documented in
> the otherwise lengthy and helpful comments near the class declarations.

Ok.
 
> --------------------
> 
> /boost/spirit/core/parser_context.hpp
> 
> // parser_context_helper and parser_scanner_linker classes
> 
> should be
> 
> // parser_context_linker and parser_scanner_linker classes

Ok.
 
> --------------------
> 
> /boost/spirit/core/subrule.hpp
> 
> #if defined(_MSC_VER) && !defined(__COMO_VERSION__)
> #pragma inline_depth(255)
> #pragma inline_recursion(on)
> #endif
> 
> I realize that these pragmas have a significant impact on code size/speed,
> but such global pragma don't belong here - this is something the user should
> tune.  The value of these pragmas should be clearly (loudly, even) called
> out in the documentation (along with recommended options/pragmas for other
> compilers).

I agree. This is not good.
 
> -----------------------
> 
> /boost/spirit/debug/debug_node.hpp
> 
> // The parser_context_linker and rule_scanner_helper classes are wrapped by
> a
> 
> should be
> // The parser_context_linker and parser_scanner_linker classes are wrapped
> by a

Ok.

> ------------------------
> 
> /boost/spirit/iterator/impl/msvc_ps_helper.cpp
> /boost/spirit/iterator/impl/multi_pass_msvc.hpp
> 
> Something wierd with the line endings in these two files - VS.NET shows
> blank lines between successive source lines.

Admitedly, this part is a bit rough (hastily ported from v1.3).
This will be corrected.

Again, thanks a lot!
--Joel