AMDG On 07/26/2012 12:20 PM, Karsten Ahnert wrote:
Hi,
here is my review for Steven Watanabe's TypeErase library.
First of all, I want to thank Steven for developing this library. My overall evaluation is that TypeErasue should be accepted as boost library. I did not looked very deeply into all details or the implementation but I tried to solve some problems with the library. So, I only looked at the library from the users perspective.
= 1. What is your evaluation of the design? =
The design looks clear and concise. I have only two small point:
* Why do I need to specify relaxed_match to get a default constructable any?
Except for the absolutely essential components, (e.g. binding_of, concept_of, placeholder_of, any -> any conversions), all behavior of an any must be specified explicitly. I bundled a lot of functionality into relaxed match. (the default constructor, assignment in terms of copy_constructible). I could split it up, if it's needed, but I assumed that all these things that make any act more like a normal object are likely to be used together.
Maybe a empty() function would be nice to check if any holds a value (in case it is default constructable).
Okay.
I think a hint about a default constructable any should be in the tutorial. It is important for users.
* Sometimes it is not clear, why some constructors work and others not, at least for me. Details are shown below.
= 2. What is your evaluation of the implementation? =
I did not looked into the implementation.
= 3. What is your evaluation of the documentation? =
* The documentation looks good. But I think more real-world examples would be very useful. I also encountered some problems where I could not find a hint how to solve it. For example, consider the case where you need an any (for example in a recursive data structure) to be a member of some other class:
struct my_class {
private: any_type a; };
What kind of constructor do I need to set up this any properly? I came up to define two constructors:
struct my_class { template< class T > my_class( T t ) : a( std::move( t ) ) { } my_class( any_type a_ ) : a( a_ ) { } private: any_type a; };
I'm not sure why you need this. Either constructor should be sufficient.
template< class Context > class not_ : struct unary_expr< bool , Context > { template< class T > not_( T child ) : unary_expr< bool , Context >( child ) { } bool operator()( Context c ) { return ! this->m_child( c ); } };
I think this is ill-formed with Concept = void.
This works quite nice, but I always had problems to figure out how the any type are constructed, for example
using expr_type = boolean_expr< void >; using not_ = not_< void >; using and_ = and_< void >; using or_ = or_< void >;
true_ t; false_ f;
// why does this not work? // expr_type e3( not_( f ) ); // cout << e3() << endl;
// why does this work? expr_type e4( and_( t , or_( f , t ) ) ); cout << e4() << endl;
The first example expr_type e3( not_( f ) ) does not compile. It fails because not_( f ) is interpreted as a function pointer:
e3 is a function declaration. It's equivalent to: expr_type e3(not_ f);
rule_engine.cpp:35:16: error: too few arguments to function ‘expr_type e3(not_)’ rule_engine.cpp:34:15: note: declared here
I have no idea why the second one compiles.
e4 can't be parsed as a function declaration. In Christ, Steven Watanabe