Re: [boost] [Review] Review of the Accumulators library begins today Jan 29

From: Steven Watanabe (steven_at_[hidden])
Date: 2007-01-30 17:29:41

AMDG

Eric Niebler wrote:
> Hi, Steven,
>
> Steven Watanabe wrote:
>   
>> accumulators.qbk lines 35-37
>> [def _mpl_                   [@../../libs/mpl MPL]]
>> [def _mpl_lambda_expression_ 
>> [@../../libs/mpl/doc/refmanual/lambda-expression.html MPL Lambda 
>> Expression]]
>> [def _parameters_            [@../../libs/parameters Boost.Parameters]]
>> these links are bad
>>     
>
>
> Thanks. I think I was overly-optimistic when I wrote that. The links 
> would work if Accumulators were in it's rightful (a-hem) place in the 
> Boost tree. ;-)
>   
unless I missed something
libs/accumulators/doc/../../libs/ == libs/libs/
Which I think is not what you intend.  Somehow in the user_s_guide.html this
becomes libs/accumulators/libs/ I'm not quite sure whether
this is a quickbook bug or the file was built as though it were in
libs/accumulators/doc/html/
>   
>> post_construct()
>> Unless for some obscure reason that I am missing it is just
>> too difficult, I would strongly prefer to make sure the constructors
>> are called in the correct order.
>>
>> The whole dance with fusion::vector is unnecessary.
>> It would be simpler to use
>>     struct accumulator_tuple_base {
>>         template<class Args>
>>         accumulator_tuple(const Args&) {}
>>     }
>>     template<class T, class Base>
>>     struct accumulator_tuple : T, Base {
>>         template<class Args>
>>         accumulator_tuple(const Args& args) : T(args), Base(args) {}
>>     };
>> Since every accumulator is put before all the
>> accumulators that depend on it in make_accumulator_tuple
>> you can write
>>     typedef
>>         typename mpl::reverse_fold<
>>             sorted_accumulator_vector
>>           , accumulator_tuple_base
>>           , accumulator_tuple<mpl::_2, mpl::_1>
>>         >::type
>>     type;
>> Then every accumulator is constructed after any
>> others that it depends on. As a bonus the only
>> limit to the number of accumulators is whatever
>> the compiler can handle.
>>     
>
>
>
> This would be a valid design choice, but it would involve reinventing a 
> lot of Fusion. I use Fusion both to generate the accumulator set, to 
> process the accumulators, and to find accumulators that match Fusion 
> predicates. Code reuse is good.
>   
I'm not suggesting that you reinvent the
fusion algorithms.  I just think that fusion::vector
is not necessarily a good sequence to use
when the constructor parameters are all identical.
Also if you write the sequence then you can
guarantee the order of initialization.

template<class From, class To>
struct match_cv : mpl::if_<is_const<From>, const To, To> {};
struct accumulator_set_tag {};
struct accumulator_set_impl_base {
    typedef accumulator_set_tag fusion_tag;
    template<class Args>
    accumulator_set_impl(const Args&) {}
}
template<class T, class Base>
struct accumulator_set_impl : T, Base {
    typedef T value_type;
    typedef Base next_type;
    accumulator_set_impl(const accumulator_set_impl& other) :
        T(static_cast<const T&>(other)), Base(static_cast<Base&>(other)) {}
    template<class Args>
    accumulator_set_impl(const Args& args) : T(args), Base(args) {}
};
struct accumulator_set_iterator_tag {};
template<class Set>
struct accumulator_set_iterator {
    typedef accumulator_set_iterator_tag fusion_tag;
    accumulator_set_iterator(Set& s) : set(s) {}
    Set& set;
};
template<>
struct boost::fusion::extension::begin_impl<accumulator_set_tag> {
    template<class Set>
    struct apply {
       typedef accumulator_set_iterator<Set> type;
       type call(Set& set) {
           return(type(set));
       }
    };
};
template<>
struct boost::fusion::extension::end_impl<accumulator_set_tag> {
    template<class Set>
    struct apply {
       typedef accumulator_set_iterator<typename match_cv<Set, 
accumulator_set_impl_base>::type> type;
       type call(Set& set) {
           return(type(set));
       }
    };
};
template<>
struct boost::fusion::extension::next_impl<accumulator_set_iterator_tag> {
    template<class Iterator>
    struct apply {
        typedef typename T::set_type set_type;
        typedef accumulator_set_iterator<
            typename match_cv<set_type, typename set_type::next_type>::type&
        > type;
        type call(Set& set) {
            return(set.set);
        }
    };
};
template<>
struct boost::fusion::extension::deref_impl<accumulator_set_iterator_tag> {
    template<class Iterator>
    struct apply {
       typedef typename T::set_type set_type;
       typedef typename match_cv<set_type, typename 
set_type::value_type>::type& type;
       type call(Set& set) {
           return(static_cast<type>(set.set));
       }
    };
};
> Regarding post_process(), I decided at the time, after discussing this 
> with Joel de Guzman (of Fusion) and Dave Abrahams, that relying on the 
> construction order of accumulators within a set would be a mistake. It 
> would rule out size optimizations like compressed_pair, where empty 
> accumulators are optimized away via the Empty Base Optimization. Fusion 
> doesn't do this optimization today, but it may in the future.
>   
But EBCO can't help you anyway.  Two distinct sub objects
of the same type (accumulator_base) cannot share the same address.
I assumed that you didn't want to rely on fusion initializing the elements
in order, that was part of the reason I suggested rolling your own fusion
sequence.
>
>   
>> What happens if you freeze a feature that another feature depends on?
>>     #include <iostream>
>>     #include <boost/accumulators/accumulators.hpp>
>>     #include <boost/accumulators/statistics/mean.hpp>
>>     #include <boost/accumulators/statistics/sum.hpp>
>>
>>     using namespace boost::accumulators;
>>
>>     int main() {
>>         accumulator_set<double, features<tag::mean, droppable<tag::sum> 
>>  > > accum_set;
>>         accum_set(1.0);
>>         accum_set(2.0);
>>         accum_set.drop<tag::sum>();
>>         accum_set(3.0);
>>         std::cout << mean(accum_set) << std::endl;  //prints 1
>>     }
>> This behavior can easily cause surprises. It must
>> be fixed to print 2.  I'm not quite sure how to implement
>> it correctly. I think that probably you need to store the
>> result at the point that drop is called for external lookup
>> and continue to collect data for internal usage. Of course,
>> this only applies in the face of dependencies.
>>     
>
>
> The code is doing something a little unusual, but the result looks 
> correct to me. What do you think the result should be, and why?
>   
The result should be 2 because 1.0 + 2.0 + 3.0 / 3 = 2.0
Users of particular accumulators should not have to care about the
internal dependencies. The mean should return the mean of all the
values that have been added regardless of whether count/sum have
been dropped.
>   
>> accumulators_fwd.hpp #define's
>> FUSION_MAX_VECTOR_SIZE/FUSION_MAX_TUPLE_SIZE
>> This is a very bad idea as it might make
>> make the value smaller than it would otherwise
>> have been. Please just check FUSION_MAX_VECTOR_SIZE
>> and issue a #error directive if it is too small.
>>     
>
>
> I had it that way at first, but changed it due to user feedback. The 
> idea is to make it so that you can just #define 
> BOOST_ACCUMULATORS_MAX_FEATURES and not have to worry about configuring 
> Fusion.
>
>
>   

Consider.

file a.cpp
    #include <boost/accumulators/accumulators.hpp>
    #include <boost/fusion/sequence/container/vector.hpp>
file b.cpp
    #include <boost/fusion/sequence/container/vector.hpp>
    #include <boost/accumulators/accumulators.hpp>

I'm not absolutely certain that this actually causes a problem
with ODR, but even if it doesn't now it is very fragile.

>
>> The docs need to state clearly what happens when
>> more than one accumulator maps to the same feature.
>>     
>
>
> Right. The first accumulator with a particular feature is the one that 
> gets used. Any others with the same feature are ignored.
>   
And the ones that are specified in the sequence are processed before those
found indirectly.

In Christ,
Steven Watanabe