Re: [boost] Proto review

From: Martin Bonner (Martin.Bonner_at_[hidden])
Date: 2008-03-14 06:32:53

From: Eric Niebler 

> Steven Watanabe wrote:

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

??
The New Collins Concise English Dictionary (1982) by my desk has:

Negate: 1. to nullify; invalidate. 2. to contradict

Posit: 1. to assume or put forward as fact or the factual basis for an
argument.  2. to put in position.

I don't really see how you can say that "to posit" is the opposite of
"to negate".

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

-- 
Martin Bonner
Senior Software Engineer/Team Leader
PI SHURLOK LTD
Telephone: [phone] / 203894 (direct)
Fax: [phone]
Email: martin.bonner_at_[hidden]
www.pi-shurlok.com
disclaimer