[boost] Proto review

From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2008-03-13 22:56:09

AMDG

*I vote to accept proto*

Documentation

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

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.

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

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?

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.
 ternary ?:
You can't overload that operator!?
 nary function call
n-ary function call?

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

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.

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?
Do transforms work if the grammar has transforms at any level?  Or do
I need a transform at the top level?

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?

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

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

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?

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?

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

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?

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

boost\proto\context\callable_eval.html:
 context(Expr::proto_tag(), arg_c\<0\>(expr), arg_c\<1\>(expr), ...)
s/\//
Why does callable_eval have no members listed?

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

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

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

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

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

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)

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.

boost\proto\exprns_\expr_Tag,Args,0__id530744.html:
operator= is specified as returning the same type!?
Is this a doxygen or boostbook problem?
 static expr::make...  What are the overloads that take
a size_t template parameter for?

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

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

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

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

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.

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

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?

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
This should include detail/dont_care.hpp
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.

deep_copy.hpp:
Ought to #include <boost/mpl/if.hpp>
The global function object proto::deep_copy can
cause the usual subtle ODR violation for objects in headers
with internal linkage.
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
#include <boost/proto/generate.hpp>

eval.hpp:
No comments.  (Yeah!)

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

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

fusion.hpp:
Needs to #include <boost/mpl/if.hpp>
Why do you put BOOST_PROTO_CALLABLE() in the
definitions of functional::flatten/pop_front/reverse
and then specialize is_callable too?
The specializations of fusion::extension::is_view_impl
use Iterator as the name of the template parameter of apply??
Why does value_of_impl<proto::tag::proto_expr_iterator> have
a specialization of apply for references?  The primary template
works, doesn't it?

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?
Is there a particular reason for not defining a generator as a
polymorphic function object?  Efficiency?
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...

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.
BOOST_PROTO_AT_TYPE(...) why add_const?
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.

matches.hpp
Should include <boost/type_traits/is_array.hpp>
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 think that you are testing the element that is in
the same position as the vararg<> in both matches<> and 
vararg_matches<>, though?
Why is matches_ specialized to return false for a terminal tag
and proto::_?

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.

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

tags.hpp:
no comments.

traits.hpp:
I don't see what BOOST_MPL_AUX_LAMBDA_ARITY_PARAM in detail::callable is 
for.
Why does as_expr not copy arrays?  It differs from deep_copy in this?
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...

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.

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?

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

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

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

In Christ,
Steven Watanabe