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