Re: [boost] Proto review

From: Eric Niebler (eric_at_[hidden])
Date: 2008-03-14 02:16:44

Steven Watanabe wrote:
> AMDG
> 
> *I vote to accept proto*

Thanks.

> Documentation
> 
> General:
> Do you think that you could use tag_of<> or ::proto_tag consistently?

I think in my code I consistently use ::proto_tag because it incurs one 
fewer template instantiation. tag_of<> is only provided for the times 
when you need a metafunction; e.g., in transforms.

> users_guide\expression_construction.html
>  The actual type of _1 looks like this:
>  expr< tag::terminal, args0< placeholder1 >, 0 >
> Should this be args1< placeholder1 >?
> Ok.  No.  I'm not really happy with this, but I don't see a better way.
> I guess the best way to think of it is that the number represents
> the number of proto expressions children rather than the
> number of arguments.

Are you concerned with the "0" in args0? It represents the arity of the 
expression.

> users_guide\expression_construction\operator_overloads.html
>  Note
>   The expr<> struct lives in the boost::proto namespace, as do all of 
> Proto's operator overloads.
>   The overloads are found via ADL (Argument-Dependent Lookup). That is 
> why expressions must be "tainted"
>   with Proto-ness for Proto to be able to build trees out of expressions.
> Is ADL the only reason?  I should hope that the operators are overloaded 
> to take expr<>.

They don't. The reason is because they should also work for user-defined 
types that extend Proto expressions. These do not necessarily inherit 
from expr<>. The operator overloads are suitably SFINAE'd so they won't 
gobble up anything they shouldn't.

> users_guide\expression_construction\left_right_arg.html
>  flatten() returns a view which makes a tree appear as a flat Fusion 
> sequence.
>  If the top-most node has a tag type T, then the elements of the flattened
>  sequence are the children nodes that do not have tag type T
> Is there a way to customize the decision of which nodes to expand?

There isn't.

> users_guide\expression_construction\tags_and_meta_functions.html
>  tag::posit:
> I don't like this name.  It confused me at first since
> posit is a complete word which means something totally different.
> positive would be better. IMO.

My (screwy?) logic is as follows: unary minus is called "negate" because 
there is a std::negate function object that applies unary minus. And 
what is the opposite of the verb "negate"? It's "posit".

>  ternary ?:
> You can't overload that operator!?

So? That doesn't mean I can't have a tag for it and process it in 
default_context with ?: as if it had been overloaded. Proto provides an 
if_else() ternary function, which behaves as the ?: operator.

>  nary function call
> n-ary function call?

operator(). Yes, should I hyphenate?

> users_guide\expression_introspection\if_and_not.html:
>  if_< is_same< _arg, char const * >() >
> Why ()?  Why a transform? Is this boost::is_same?

Yes, this is boost::is_same. What else would I use there aside from a 
transform?

> users_guide\expression_introspection\defining_dsel_grammars.html:
>  We can use matches<> and BOOST_MPL_ASSERT() to issue readable 
> compile-time errors for invalid expressions, as below:
> I'm only partially convinced.  The error message will be short, sure,
> but if the expression is complicated, it may not be enough
> information, since this a very coarse grained error message.

It would be nice to be able to tell people *why* their pattern doesn't 
match the grammar. It's a hard problem and I haven't figured it out yet.

It is probably better than the alternative, which would be to generate a 
cryptic error and megabytes of spew. For those willing to wade through 
the spew and examine the library source, it /might/ provide more context 
to understand the error. Those people are few, IMO.

> users_guide\expression_transformation.html:
>  A grammar decorated with transforms is a function object that takes 
> three parameters:
>   expr -- the Proto expression to transform
>   state -- the initial state of the transformation
>   visitor -- any optional mutable state information
> What type should "state" and "visitor" be?  Why
> are they two different arguments?

Whatever you want. There are two different parameters because some 
transforms (e.g., fold) will update the state parameter automatically. 
Sometimes you want to pass around a mutable blob that only gets modified 
by you, not by fold. That's when you use a visitor.

The Haskell "Scrap Your Boilerplate" technique (aka, The Mother of All 
Traversals) has similar parameters. The state is called an 
"accumulation" and the visitor is called the "context". I designed this 
part of Proto before I knew about the Haskell work. I think it's 
interesting that we converged on the same functional specification.

> Do transforms work if the grammar has transforms at any level?  Or do
> I need a transform at the top level?

All grammar elements have a default transform. So if the default on the 
top level is acceptable to you, you don't need to specify it.

> users_guide\expression_transformation\canned_transforms\if.html:
>  The result of applying the first transform should be a compile-time 
> Boolean. If it is true...
> Meaning it should be an MPL Integral Constant Wrapper?  If non-zero?

Yes.

> users_guide\expression_transformation\canned_transforms\make.html
>  This uses proto:::make<> under the covers to evaluate the transform.)
> s/:::/::/

Thanks.

> users_guide\expression_transformation\canned_transforms\when.html
>  that it, when<> also behaves like a transform.
> s/it/is/

Thanks.

> users_guide\expression_transformation\canned_transforms\fold_tree.html:
>  fusion::cons<FoldTreeToList, _state>(FoldTreeToList, _state)
> I'm getting confused.  Let me see...  This is a transform that
> applies FoldTreeToList to its argument and uses the result to construct
> a fusion::cons with the current state.  Question.  Why reverse_fold_tree?
> would reverse_fold work as well because it would first evaluate the
> right side of the comma, creating a fusion::cons and then take the left
> side and push it onto the front?

I think you'd end up with a cons<cons<>, cons<> >, which is not what you 
want. Try it and see!

> users_guide\expression_transformation\is_callable.html:
>   ...a problem for a type like std::vector<_arg(_arg1)>(), which is a 
> valid transform...
> Can't you substitute /first/ and /then/ check is_callable?

I could do that, yes. And I seem to recall having a reason for not doing 
that.

... time passes ...

Either the reason escapes me, or else my reasoning was wrong. I'll 
investigate this.

> users_guide\expression_extension\extends.html:
> What do I do if I want a statically initialized terminal
> that has a custom domain?  Inhetitance from extends<>
> makes my type a non-aggregate...

Right, there is a BOOST_PROTO_EXTENDS() macro for this, but it's already 
been noted that is isn't documented.

> It looks like the comments in users_guide\examples\mixed.html in
> DereferenceCtx and IncrementCtx were copied from vector.html and
> not updated.  Also, VectorOps should be MixedOps?

Thanks.

> boost\proto\args\hpp.html
> Why is args0 unary!?

I'm sorry, where are you looking? args0<> does have one element in it: 
the type of the terminal. It has 0 child expressions, though, so its 
arity is really zero.

> boost\proto\context\callable_eval.html:
>  context(Expr::proto_tag(), arg_c\<0\>(expr), arg_c\<1\>(expr), ...)
> s/\//

Thanks.

> Why does callable_eval have no members listed?

Because the primary template is indeed empty. Only the specializations 
have members.

> boost\proto\context\callable_context.html:
> Is it just more boostbook weirdness that the Synopsis
> has an empty eval<>?

Yes, eval<> has a base that isn't showing up. BoostBook bug. Crud.

> boost\proto\context\callable\hpp.html:
> Do you really need to show all the specializations
> of callable_eval?

Probably not, but getting something else would involve waging a costly 
war with our documentation toolchain.

> boost\proto\context\null\hpp.html
> ditto.

Right.

> boost\proto\display_expr_id535716.html:
> The one argument form writes to std::cout, right?

Yes, noted.

> boost\proto\domainns_\domain.html:
>  boost::proto::domainns_::domain
> Is domainns_ a private namespace to block ADL?
> If so, it should not appear in the documentation...

You're right. More battles with the documentation toolchain. :-(

> boost\proto\functional\eval.html:
> A general comment about eval: It should be noted
> that the context is *not* copied, and so it
> is safe for it to have mutable state.  (Since
> this is not usual for function objects which
> is what callable_context implements)

True.

> boost\proto\expr\hpp.html:
> I don't think that you should need to show all the
> specializations of expr in the documentation.
> exprns_ is a private namespace?  It shouldn't appear
> in the documentation.

I guess. :-/ The fact that these types live in special namespaces is a 
part of the interface, in a way, in that it avoid ADL problems. I'm of 
two minds about this.

> boost\proto\exprns_\expr_Tag,Args,0__id530744.html:
> operator= is specified as returning the same type!?
> Is this a doxygen or boostbook problem?

Doxygen or BoostBook, I don't know.

>  static expr::make...  What are the overloads that take
> a size_t template parameter for?

So that you can initialize a terminal<int[4]>::type -- that is, you can 
have a terminal that holds an array by value.

> boost\proto\fusion\hpp.html
> What is the rational for only bringing flatten into namespace proto.

There's no sense having proto::pop_front() and proto::reverse(). They 
would do the same thing as fusion::pop_front() and fusion::reverse(). I 
wouldn't even need these if Fusion provided function object equivalents 
of its free functions.

> boost\proto\is_extension.html:
> What does this do?

Another piece that fell through the cracks. It lets you non-intrusively 
adapt a type to be a Proto terminal, as long as you bring the 
proto::exops namespace into scope (which is also undocumented, argh!).

> boost\proto\operators\hpp.html:
> Why does if_else specify the return type while all the
> others say /unspecified/

An implementation artifact. The binary operators do not have their 
return type expressed in terms of make_expr<> because it would incur 
unnecessary template instantiations. That's tractable with only 2 
argumemts. With 3, the overload set and return type calculation became 
too tedious to write out, so I just used make_expr<> and required the 
parameters to be const&. Not ideal, but also not a big deal, IMO.

> boost\proto\proto_fwd\hpp.html:
> this is not pretty.

It's a horrible mess. Doxygen doesn't emit documentation for forward 
declarations, but it does for typedefs. Not sure what to do about it. :-/

> boost\proto\transform\fold\hpp.html:
> Why does this say that the it contains fold_tree, but fold_tree
> doesn't appear in the listing.

I can't imagine why. The comment from fold_tree.hpp seems to have 
mysteriously migrated there. Ah! In fold_tree.hpp, I have mistakenly 
labeled it with \file fold.hpp. Fixed.

> boost\proto\transform\reverse_fold.html:
> This says that reverse_fold simply inherits from fold!?

[sigh] I know. reverse_fold really inherits from 
fold<call<_reverse(Sequence)>, State0, Fun>. I tried everything to get 
it to show up correctly, but lost that battle with Doxygen. Really, 
sometimes I wonder if Doxygen is more trouble than it's worth. At other 
times, I'm sure it is.

> boost\proto\transform\fold_tree.html:
> Is there some way to do a fold tree that recurses over
> some set of tags rather than a single fixed tag?

No, but it wouldn't be difficult to define. fold_tree's implementation 
is dead simple.

> boost\proto\transform\make.html:
>  Function pointer types
> How are function pointer types interpreted as transforms?
> Oh. OK.  They are treated the same as functions.
> 
> Source:
> 
> args.hpp:
> No comments.  (Besides what I said about arg0 earlier)
> 
> context.hpp:
> no comments.
> 
> debug.hpp:
> why doesn't this use detail/prefix.hpp and suffix.hpp

It probably should.

> This should include detail/dont_care.hpp

Right.

> Why does the display_expr take the depth first?
> I would expect that users would want to override the
> default output stream more often then the starting depth.

You might be right. I have no strong feelings.

> deep_copy.hpp:
> Ought to #include <boost/mpl/if.hpp>

Right.

> The global function object proto::deep_copy can
> cause the usual subtle ODR violation for objects in headers
> with internal linkage.

Right. It should be a function, anyway, so ADL can find it.

> I don't think that this header needs to #include <boost/proto/generate.hpp>
> 
> domain.hpp:
> This should #include <boost/ref.hpp> and does not need to

Right.

> #include <boost/proto/generate.hpp>

Actually it needs this one because one of domain<>'s template parameters 
defaults to default_generator, defined in generate.hpp. People should be 
able to use domain<> without having to separately #include generate.hpp.

> eval.hpp:
> No comments.  (Yeah!)

Whew.

> expr.hpp:
> Why is operator= only overloaded for non-const expr's when
> they are terminals?
> Same thing for operator[]

IMO, they're unlikely to ever be used and leaving them out measurably 
improves compile times. They would only conceivably be used in 
expression like (a+b)=x or (a+b)[x]. However, Proto's operator overloads 
return const rvalues, so a non-const operator= or operator[] on a 
non-terminal would never even be considered. Ditto for operator(), where 
leaving the const overloads out has a dramatic effect on compile times.

> Doesn't use #include <boost/preprocessor/arithmetic/inc.hpp>
> nor #include <boost/preprocessor/facilities/intercept.hpp>
> You're not using #include <boost/utility/result_of.hpp>

Thanks.

> extends.hpp:
> needs to #include 
> <boost/preprocessor/repetition/enum_trailing_params.hpp> and
> <boost/preprocessor/repitition/repeat_from_to.hpp>.  Does not need
> <boost/preprocessor/arithmetic/dec.hpp> or 
> <boost/preprocessor/facilities/intercept.hpp>

Agreed, except for dec.hpp.

> Should there be a BOOST_PROTO_EXTENDS_ALL macro, so that I don't
> have to write out all four macros?  At the very least, you need to
> note that BOOST_PROTO_EXTENDS does not overload [], (), and =, since
> it differs from boost::proto_extends in this respect.

Right, I might rename BOOST_PROTO_EXTENDS to BOOST_PROTO_BASIC_EXTENDS, 
and have BOOST_PROTO_EXTENDS mean the same thing as extends<>. And 
document it. :-/

> fusion.hpp:
> Needs to #include <boost/mpl/if.hpp>

Gotcha.

> Why do you put BOOST_PROTO_CALLABLE() in the
> definitions of functional::flatten/pop_front/reverse
> and then specialize is_callable too?

You're right, I don't have to. The is_callable specialization shortcuts 
a few template instantiations, tho. And BOOST_PROTO_CALLABLE() is so 
that any types derived from flatten/pop_front/reverse are also callable. 
(As if anybody would want to do that.)

> The specializations of fusion::extension::is_view_impl
> use Iterator as the name of the template parameter of apply??

Typo.

> Why does value_of_impl<proto::tag::proto_expr_iterator> have
> a specialization of apply for references?  The primary template
> works, doesn't it?

Yup.

> generate.hpp:
> What is the point of expr_traits?  Doesn't expr expose this
> information directly?  It is in namespace detail so it isn't
> intended to be specialized by users is it?

Correct. I have to be very careful here not to do anything which could 
instantiate the expression type here. There are places in the code where 
the return type of an expression's member function is expressed in terms 
of generate, which is expressed in terms of the expression type, etc... 
Loopy compile problems.

> Is there a particular reason for not defining a generator as a
> polymorphic function object?  Efficiency?

No good reason. I should change it.

> The template template arguments for pod_generator are worrisome.
> Take a look at the example in extends.hpp for is_proto_expr.
> If I want to make a POD wrapper it seems that I need /two/
> template arguments and thus cannot use pod_generator at all...

Well, in that example, my_terminal<> is a terminal wrapper, not a 
general expression wrapper, so you wouldn't pass it to pod_generator. I 
understand your concern, but I don't want to use something like an mpl 
placeholder expression here because of overhead. The generator protocol 
is so dead simple that is generate<> and pod_generate<> don't mean your 
special needs, it takes ~2 minutes to bang together a custom generator 
that does.

> make_expr.hpp:
> You might note the format of DATA in the macros.  I'm
> finding it difficult to work out what they are doing.

What's your objection?

> BOOST_PROTO_AT_TYPE(...) why add_const?

If fusion::value_at<> returns a reference, add_const is a no-op. 
Otherwise, it lets me pass rvalues. IIRC.

> The implementation of BOOST_PROTO_VARARG_TEMPLATE_YES_ converting
> a seq to a tuple to a list is clumsy.  Why isn't it implemented using
> BOOST_PP_SEQ_FOR_EACH_I_R?  Same thing goes for 
> BOOST_PROTO_TEMPLATE_PARAMS_YES_
> What is BOOST_PROTO_SEQ_PUSH_FRONT for?  Oh.  I get it.
> This is already being instantiated inside a BOOST_PP_SEQ_FOR_EACH_I...
> I'm thinking that these macros belong in their own file.

It's all very complicated. I took me days to write those stupid macros. 
Please don't make me look at them again -- they make my eyes bleed. The 
best that can be said about them is that they work. I confess I'm a 
terrible PP metaprogrammer.

> matches.hpp
> Should include <boost/type_traits/is_array.hpp>

Good catch. How do you do that?

> I don't quite get vararg_matches.  The specialization for Zero = false
> looks fishy.  If I understand correctly, you are explicitly
> forcing the size to /include/ the vararg argument.
> In fact, I'm not sure that I like the fact that you are forcibly
> overriding the size of the arg sequence at the expr level at all.
> In addition, the argument "From" to vararg_matches_impl will skip
> over the argument that corresponds to the vararg.  What am I missing?
> Oh.  I see, vararg_matches_impl doesn't take a half-open range...
> Is there a noticeable performance improvement vs. doing this more
> naturally?  Ok it looks right, but it took quite a bit of work to
> demonstrate.  

I have sunk weeks into the implementation of proto::matches<> to make it 
as fast as possible. I play some dirty pool in there, I know. I think 
it's all kosher, tho.

> I think that you are testing the element that is in
> the same position as the vararg<> in both matches<> and 
> vararg_matches<>, though?

Really, where? I don't see that. Keep in mind that template 
instantiations are memoized by the compiler. Asking for the same 
instantiation twice is "free".

> Why is matches_ specialized to return false for a terminal tag
> and proto::_?

Huh! That doesn't look right, does it? Could be a relic of the time when 
terminals were degenerate unary expressions.

> operators.hpp:
> no comments.
> 
> proto.hpp:
> no comments.
> 
> proto_fwd.hpp:
> Why do you hard code arg0, arg1, ... rather than using the
> preprocessor?  I know it's easy but it's still not configurable.

That's pretty dumb of me. I'll fix it.

> proto_typeof.hpp:
> no comments.
> 
> ref.hpp:
> why does unref leave array references as references, but does
> not treat plain arrays differently from other values?

You're right, I'm being a bit schizo about array types. The question is, 
does deep_copy cause arrays to be stored by value? I worry about immense 
amounts of data getting copied all over. But maybe the answer is yes, it 
probably should. A simple reference wrapper avoids the copy in that 
case. At the very least, I need to be consistent.

> In other files you create an object of type functional::*
> for unref you duplicate the overloads at namespace scope.
> It would be best to stick to a single style consistantly, IMO.

Yes, they should all be free functions, so ADL can find them.

> tags.hpp:
> no comments.
> 
> traits.hpp:
> I don't see what BOOST_MPL_AUX_LAMBDA_ARITY_PARAM in detail::callable is 
> for.

It's for working around gcc bugs.

> Why does as_expr not copy arrays?  It differs from deep_copy in this?

More schizo array weirdness. Will fix.

> All the operator definitions are the same thing over and over and over
> again.  Why don't you use a macro?  Oh.  Except the documentation 
> comments...

Yes, it's for Doxygen.

> context/callable.hpp:
> no comments.
> 
> context/default.hpp
> no comments.
> 
> context/null.hpp
> no comments.
> 
> detail/as_lvalue.hpp
> The name uncv is misleading since it is only unconst.

It's a detail, but ok.

> detail/dont_care.hpp
> no comments.
> 
> detail/funop.hpp:
> no comments.
> 
> detail/pop_front.hpp:
> no comments.
> 
> prefix.hpp:
> no comments.
> 
> reverse.hpp:
> no comments.
> 
> suffix.hpp:
> no comments.
> 
> transform/arg.hpp:
> why does transform::_ref have a leading _ unlike expr?

Because if it were just "ref" then, with gcc's strange name look-up 
implementation, it would be ambiguous with the boost::ref() function. 
Crazy and stupid, but not my fault.

> transform/bind.hpp:
> no comments.
> 
> transform/fold.hpp:
> I don't think that detail::as_callable is a good name.
> It should be more like: bind_visitor_and_transform_for_fusion_fold

Ha! A bit verbose, perhaps...

> transform/fold_tree.hpp:
> no comments.
> 
> transform/make.hpp:
> Why are there specializations of construct_ for expr both
> for any number of arguments and for each number of arguments?

Damned if I know. :-D I'll fix it.

> The documentation says that
>   If X is a template like Object2<Y0,Y1,...>, ...
>   Otherwise, if X is a transform ...
> The tests actually happen in the opposite order.
> In other words, a templated transform is treated as a transform,
> not a template.

Great catch. Looks like a documentation bug, I think the code is doing 
the right thing.

> transform/pass_through.hpp:
> no comments.
> 
> transform/when.hpp
> no comments.
> 
> General:
> 
> It would probably better to define a macro BOOST_PROTO_HAS_SEPARATE_FUSION
> rather than constantly saying #if BOOST_VERSION < 103500

Why is that better?

You obviously put a huge effort into this review. I'm deeply grateful 
for your contribution. Thank you.

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