Re: [boost] Proto review

From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2008-03-14 12:39:07

AMDG

Eric Niebler wrote:
>> 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.
>   
Ok.  I'm happy as long as there is some defense against wild overloads 
that match anything.

>   
>> 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.
>   
I think I understand now.

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

It looks like doxygen/boostbook is getting confused here, because the
fact that the argument is an array doesn't show up.

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

Ok.  Anyway, the const overloads will work for non-const arguments.

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

Right.  I must have searched for dec rather than DEC...

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

Ok.  As long as it's documented what the interface I have to meet is.

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

Just that a few comments would have made it much easier to follow.

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

Ok.  I'll suggest though that vararg_matches_impl should take
a closed range i.e. [1, 3] to check the arguments 1, 2, and 3
rather than offsetting it by one [2, 4] to match arguments 1, 2, and 3.

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

Never mind.  I was confused.

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

Just a general prejudice against "magic" numbers

In Christ,
Steven Watanabe