Re: [boost] [type_erasure] Review started (July 18-27, 2012)

Subject: Re: [boost] [type_erasure] Review started (July 18-27, 2012)
From: Larry Evans (cppljevans_at_[hidden])
Date: 2012-07-28 06:52:53

On 07/27/12 15:19, Steven Watanabe wrote:
> AMDG
>
> On 07/27/2012 11:14 AM, Julien Nitard wrote:
>>
>> Please find my review of Type Erasure below.
>>
>>
>>> Please state clearly whether you think this library should be accepted
>>> as a Boost library.
>>
>> Yes.
>>
>>> 1. What is your evaluation of the design?
>>
>> The library is easy to use, at least for the use cases I am
interested in.
>> Still it provides what I consider to be very very advanced features.
>>
>> Though, I'd question the need for some those very advanced
functionalities.
>> Will many people use something has cumbersome as :
>>
>> tuple<requirements, _a, _b> t(&array[0], 2);
>> any<requirements, _a> x(get<0>(t) + get<1>(t));
>>
>
> The main benefit for this is for type
> erased function arguments:
>
> typedef mpl::vector<
>   random_access_iterator<_iter>,
>   same_type<random_access_iterator<_iter>::value_type, _a>,
>   less_than_comparable<_a>,
>   move_constructible<_a>,
>   move_assignable<_a>,
>   callable<bool(const _a&, const _a&), _f>,
>   copy_constructible<_f>
>> sort_requirements;
>
> void sort_impl(iter_type, iter_type, func_type);
>
> // capture the arguments and forward to the
> // separately compiled implementation
> template<class Iter, class F>
> void sort(Iter, Iter, F);

What does "capture the arguments" here mean?
I'd guess it means substituting some actual arguments
types for the placeholders and actual argument values
for the, I guess you'd call them, contained types,
but I don't see any actual argument values in the
example; hence, that guess doesn't fit.

Could you please elaborate on this example?

>
[snip]
>>> 2. What is your evaluation of the implementation?
>>
>> The implementation is nearly not documented. There are very few
>> comments in the private part of the code and there is no coding
>> guidelines. This makes a review of the implementation very hard
>> especially because a library like this is composed mostly of
>> boilerplate TMP code. I wanted to figure out how the vtable was
>> created and used, but I got discouraged. Adding some help for would be
>> hackers could be interesting.
>>
>
> The vtable isn't terribly complex.  It's essentially
> a statically initialized fusion::map.
>
> template<class T1, class T2>
> struct vtable2 {
>     typename T1::type t1 = T1::value;
>     typename T2::type t2 = T2::value;
>     typename T1::type lookup(T1*) const { return t1; }
>     typename T2::type lookup(T2*) const { return t2; }
> };
>

I've seen the T::type code a lot (I think in error messages); however,
I've never understood what the T's were.  Could elaborate on
what they are and how they're produced?

> A lot of the library internals are similar in
> that they're essentially simple components
> with a lot of preprocessor baggage around them.
> The only real algorithmic complexity is in
> normalize.hpp.

But normalize.hpp contains no in-source comments, which perfectly
illustrate Julien's remark:

  There are very few comments in the private part of the code

Like Julien, I also tried to figure out how vtable worked, which lead
me to examing normalize.hpp. The lack of in-source comments made this
very discouraging.  I finally resorted to breaking down the source and
printing the demangled types names; however, I've still not figured it
out.

I'd suggest breaking down the source, somewhat like in the attached,
attach comments to the parts explaining what they do, then creating
test cases for each part in put those unit tests in the
*/libs/type_erasure/test directory.  This would not only document the
implementation better, but provide better testing, and if the
in-source comments weren't enough, looking at the unit tests might
help future hackers figure out what the code does.

>
[snip]
>> Another thing, would be to work on error messages, because this being
>> C++ they are horrible. I don't know if static asserts may help a lot
>> easily or not, but if anything can be done, it'll be welcome.
>>
>
> I can make sure that the library doesn't
> produce deep template instantiation
> backtraces in most cases.  There are
> a few cases where a static assertion
> would help too.

What about using:

  http://www.boost.org/doc/libs/1_50_0/libs/concept_check/concept_check.htm

because, as that .htm page says:

  The Boost Concept Checking Library uses some standard C++ constructs
  to enforce early concept compliance and that provides more
  informative error messages upon non-compliance.

[snip]

-regards,
Larry