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

From: Eric Niebler (eric_at_[hidden])
Date: 2007-01-30 12:47:43

Hi, Steven,

Steven Watanabe wrote:
> AMDG

Ad Majorem Dei Gloriam?

> user_s_guide.html#id450311 line 363
> accumulators.qbk line 189
> for some reason external<> is not a link in the html

Ah, the link is broken. Thanks.

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

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

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.

> 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 user guide needs to specify what header each component is in.

Yes.

> BOOST_PARAMETER_NESTED_KEYWORD.html
> There is no description of what this macro is for.

Thanks.

> accumulator_set.hpp line 149
> corresponds to -- What?

... a feature. Fixed.

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

> For simple cases at least the library seems
> to cause little to no overhead with msvc compared to
> hard coding everything.

Thanks for the report!

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

> Overall the users guide is quite good.
> I had no difficulty understanding it.
> The reference manual is pretty sketchy though.

Good docs are always the hardest part.

> For the most part the interface is clean and easy to use.
> 
> I definitely think Accumulators should be included in boost.
> 

Thanks.

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