On 7/18/12 7:13 AM, Lorenzo Caminiti wrote:
*** The review of Steven Watanabe's proposed Boost.TypeErasure library
Please state clearly whether you think this library should be accepted as a Boost library.
A clear Yes, here.
Other questions you may want to consider: 1. What is your evaluation of the design?
Let me start by saying that this library is a very impressive piece of software. It opens up new possibilities in solving several problems in developing large scale software systems in a way I haven't seen before. I have two concerns about the design: a. downcasting. It has been mentioned in the discussion, that this would be a useful feature. Since this feature is coupled with how the any type stores the concepts and cannot be implemented without breaking encapsulation, it needs to be part of the library, or the library has to provide stable hooks for it. Steven has already bumped up the priority of this, so I am confident that this will be solved in some way. Since this feature needs quite a bit of user facing machinery a mini review for this might be needed. I do not make the inclusion of this feature a condition on acceptance, because I trust Steven to include at some time. b. creating custom concepts/concept interfaces. Consider this paragraph as dreaming out loud. I like the way Steven solved the problem of defining concepts but I still like it to be easier, because I believe that how easy it is to define concepts is the key to widespread adoption. Ideally it should not be more complex to define a concept than to define a (virtual) interface. My ideal solution would be able to do this: struct int_container_concept { //... void push_back(int) = 0; size_t size() = 0; // b.2 //... }; void legacy_func(int_container_concept& c); // b.1 BOOST_CREATE_CONCEPT(int_container_concept); void test() std::vector<int> v = /*filles somehow*/; any<int_container_concept> con = v; legacy_func(con); } I know that it is impossible to implement the BOOST_CREATE_CONCEPT macro in the above example without language support. So until we get compile time reflection in c++ we need to live with the additional boilerplate in some way. In the "Design" section Steven explains that to scrap the boilerplate he departed from defining the user facing interface using abstract classes. My concern with this departure is that it is quite different, and not directly compatible. There are two things type erasures concepts cannot do: b.1. I cannot use a concept without using any<>. (i.e. the legacy_func marked above would need to take an any<int_container_concept>& instead) This might be a deal-breaker for some users of the library. b.2. I cannot define a concept that has more than one function directly, i.e. I have to define concepts for push_back and size and then compose them (in this case 35 lines of code). This is of course mainly an issue of syntactic sugar, but in Stevens concept definition boiler plate and interface information (member name, arguments, composition lists) are intermixed. I seem to recall that there was some discussion about having some macros to simplify the boilerplate. I think something along the lines of BOOST_CREATE_CONCEPT(container_concept, typename T, (void (push_back, T), void (size)) ) wouldn't be too bad.
2. What is your evaluation of the implementation?
I only skimmed through the implementation, and found it surprisingly easy to understand for such a meta programming heavy library. Even so it could use a few comments. The Implementation is clean, yet still unoptimized. I think this is reasonable at this point in time. I trust Steven to apply optimizations as time goes by. It would be nice to see the Small Object Optimization relatively soon. The error messages are not to good, especially a "concept missmatch" spews out several rather unhelpful implementation details. (This is also already aknowledged by Steven.)
3. What is your evaluation of the documentation?
The documentation that is there is solid. As others have noted It needs more Motivation, Tutorials and Examples. I especially would like to second one point Eric Niebler made:
In short, I'd put a handful of examples at the front of the users' guide that are relate-able and illustrative. Otherwise, you just have a listing of features and sample usages, with no motivation, no rationale, and no thread tying the features together.
It might even help to describe what "type erasure" is, how it's been traditionally used, and what one of the simpler examples would look like without Boost.TypeErasure. That way, folks can get a feel for the drudge-work that your library saves them.
I think this (especially the second paragraph) is the key to making the docu more accessible.
4. What is your evaluation of the potential usefulness of the library?
Very useful, a potential game changer! It opens the door to a compelling alternative to building systems with it rather than inheritance based polymorphism. It also provides a compelling motivation for bringing "static reflection" to C++ because this feature would allow trivial definition of custom concepts and knowing which concepts a concrete class supports.
5. Did you try to use the library? With what compiler? Did you have any problems?
Build the examples with clang-3.1(Apple) and a build a small toy example myself to gain some insights. No problems there.
6. How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Deep reading of the documentation, superficial code reading, some experiments, lots of thinking. 2-3 days.
7. Are you knowledgeable about the problem domain?
Quite. Best regards, and thanks to Steven for this great library. Fabio