On 07/18/12 00:13, Lorenzo Caminiti wrote:
Hello all,
*** The review of Steven Watanabe's proposed Boost.TypeErasure library begins on July 18, 2012 and ends on July 27, 2012. *** [snip] Should type_erasure be accepted into Boost?
Answer: No, not yet. Why(in order of importance): 1) any<C,T>::any(U&, static_binding<Map>&) bug The bug reported here: http://article.gmane.org/gmane.comp.lib.boost.devel/233101 should be resolved. The bug is just too dangerous for the resolution to be delayed. 2) any<C,T>::any(U&, binding<C>const&) documentation problem. The vague documentation problem described below ( search for [vague:any<C,T>::any(U&, binding<C>const&)]:) should be resolved first. A novice user, such as myself, could easily misunderstand the purpose of this constructor and spend hours before finally stumbling upon the answer or pestering Steven for some explanation. Solving this now would probably save future novice users and probably Steven valuable time. My review: 1. What is your evaluation of the design? Good, except for: [need variadic static_binding ctor]: The any CTOR: template<class U, class Map> any(U & arg, const static_binding< Map > & binding); documented here: http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/... allows only a single argument to the constructor to the underlying type. I'd suggest adding another CTOR with interface: template<class Map, class... U> explicit any(const static_binding< Map > & binding, U &&... arg); which is similar to: explicit any(const binding< Concept > & binding, U &&... arg); Then any constructor of the underlying could be used. [too many any ctors]: According to `grep -e ' any('`, there are more than 45 any constructors. That many makes it hard for the novice to know which one to use. The Design notes: http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/... are some help, but those notes also mention ambiguities and user rules, which sound like indications of usability problems to me. Why wouldn't the following ctors be sufficent to do all the current any constructors do (except for maybe requiring more typing and maybe more constructor calls)? template < class... U > any ( static_binding<Map>const& , U&&... );//kind=binding constructor template < class... U > any ( binding<Concept>const& , U&&... );//kind=dispatching constructor template < class Concept2 , class Tag2 > any ( any<Concept2,Tag2>U&& , static_binding<Map>const& );//kind=converting constructor template < class Concept2 , class Tag2 > any ( any<Concept2,Tag2>U&& u , binding<Concept2>const&b=binding_of(u) );//kind=converting constructor Providing just these 4 constructors would improve the usabilty. Of course, I'm probably missing something, and if so, explaining why the above would not work in the Design notes would be helpful. 2. What is your evaluation of the implementation? [in-source comments]: Very few in-source comments, as for example, the in the code: http://steven_watanabe.users.sourceforge.net/type_erasure/boost/type_erasure... there are no in-source comments. This lack of in-source comments made it very hard to understand the implementation. There is also no tests in the directory: http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/... which contain any string with 'normalize' in it; hence, I assume there are no unit tests for any class or function with normalize as part of its name. Such unit tests, hopefully, would have provided more information of the purpose of the code in */detail/normalize.hpp; hence, I'd recommend providing such tests in addition to more in-source comments. [any.hpp mysterious false?exp1:exp2 expr]: In any.hpp, there's several expressions like: false? this->_boost_type_erasure_deduce_constructor(arg...) : 0 Since: false?exp1:exp2 always evaluates to exp2, this just obscures the code. It should be replaced with exp2(i.e. 0) or some in-source comment attached explaining why the ?: operator is needed. 3. What is your evaluation of the documentation? Mostly good, but some documentation needs improvement. For example: [vague:rebinding_any.html]: This doc: http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/... Claims rebind_any<Any,T>::type: returns any type corresponding to a placeholder. However, it doesn't say what the Any in rebind_any<Any,T> may be, and if one assumes Any is a type_erasure::any<C,S> for some concept, C, and placeholder, S, then that description makes no sense because C does not contain any information about which actual type is matched to which placeholder. That information is only avaiable from the any<C,S> constructor arguments or from the any<C,S>::table member variable, IIUC. I would think example code illustrating the purpose might help. There is an example here: http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/... and that shows Base as the Any arg in rebind_any.html. Now since Base is a template arg to concept_interface, I have to look at the concept_interface doc here: http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/... which says Base is: The base of this class. which is little help, which forces me to search elsewhere to understand rebind_any. At this point I resorted to actually writing code print printing out demangled names to help figure out rebind_any. However, that failed to help either. At that point, I gave up. [vague:any<C,T>::any(U&, binding<C>const&)]: The ref doc for this: http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/... contains: Requires: Concept must contain a matching instance of constructible. with no definition of "matching". There is something about "relaxed_match" documented here: http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/... but that just contains: In the presence of this concept, the requirement that all the arguments to a function must match is relaxed. from which I assume that "matching" means: all the arguments to a function must match exactly. But that still does not define what "match exactly" means. It can't mean just what the c++ language standard: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3290.pdf calls "Declaration matching" in section 13.2 because a type_erasure concept includes the placeholders and surely "matching" doesn't mean a placeholder is expected as the actual argument to a function signature used in a concept definition. Now, the placeholder doc: http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/... contains: For example, in the following, any<copy_constructible<_a>, _a> x(1); The library sees that we're constructing an any that uses the _a placeholder with an int. Thus it binds _a to int and instantiates copy_constructible<int>. since, according to the source codesuggests that the concept(which is part of copy_constructible): constructible<_a(const _a&)> would allow an _a to be copy constructed from another _a, or based on the aforementioned placeholder.html page, it would an instance of the type bound to _a (call it bound_a), to be copy constructed from another instance of bound_a; however, the post here: http://article.gmane.org/gmane.comp.lib.boost.devel/233023 shows that's not the case. Only after several exchanges between I and Steven in that thread, and the hint: The library deals with either any's or with the contained types, but not both, depending on what layer you're looking at. You're trying to mix the two. from Steven, in the reply: http://article.gmane.org/gmane.comp.lib.boost.devel/233067 did I finally conclude: The placeholders in concepts do *not* stand for the bound types but instead for the any's containing those bound types. Which is similar to the conclusion which I posted here: http://article.gmane.org/gmane.comp.lib.boost.devel/233078 Since the 233078 post got no replies, I assume that conclusion is right. If so, then that should be made clear in the documentation (including tutorials). In addition, the following example code (and especially the comments), should reduce future confusion on this topic: /* {***any_ctor_kinds.cpp*** */ //Purpose: // Demonstrate the different purposes of different // "kinds" any CTOR's. //References: // [ConsKind] // http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/... #include <iostream> #include <boost/type_erasure/any.hpp> #include <boost/type_erasure/binding_of.hpp> namespace mpl = boost::mpl; using namespace boost::type_erasure; template < unsigned ThisId
struct v { }; int main() { typedef mpl::vector < copy_constructible<_a> #define COPY_CTOR_V0 #ifdef COPY_CTOR_V0 , constructible<_a(v<0>const&)> //allows y(a_binding, v0) //(see below) to compile. #endif > a_concept; typedef any < a_concept , _a > a_any; v<0> v0; a_any x ( v0 ) /** * This CTOR does no dispatching, it simply * calls the decltype(v0) CTOR with argument, v0. * * This is a binding constructor, as defined in [ConsKind]. */ ; binding<a_concept>const& a_binding=binding_of(x); a_any y ( a_binding /** * This makes this constructor a dispatching constructor, * according to item 2 in the 2nd numbered list in [ConsKind]. * * Hence, the args after this 1st arg must * match one of the concepts in a_concept. */ #define PH2VALUE #ifndef PH2VALUE , x //With this argument, this y declaration //matches concept: // constructible<_a(const _a&)> //where the 1st _a must match decltype(y) //(because that's what is being constructed), //and the 2nd _a must match decltype(x) //(because that's the only argument), //and decltype(y) == decltype(x). // //Since constructible<_a(const_a&)> //is included in copy_constructibe<_a>, //and copy_constructibe<_a> is in a_concept, //this will compile. #else , v0 //With this argument, this y declaration //does not match concept: // constructible<a_(const& _a)> //because, //but does match concept: // constructible<_a(const& v<0>)> //Hence, for this to compile, a_concept must //include the latter concept. #endif ) ; return 0; } /* }***any_ctor_kinds.cpp*** */ If that conclusion is wrong, then the docs need to be modified (I've no idea how) to clearly define "matching" as used in the any.html#id2445089-bb reference. [incomplete:tuple.html]: The doc: http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/... makes no mention of how the binding produced by this call are generated. Something like the following brief addtion would answer that question: The bindings for each any in the constructed tuple is generated from, in essense, the mpl::map: mpl::map < mpl::pair<T,U>... > For eample, with: tuple<C,T0,T1,...,Tn> t(u0,u1,...un); the mpl::map used to generate the bindings for all of: get<0>(t) get<1>(t) ... get<n>(t) would be, essentially: mpl::map < mpl::pair<T0,decltype(u0)> , mpl::pair<T1,decltype(u1)> ... , mpl::pair<Tn,decltype(un)> > [missing:tutorial needs some Constructors Design notes]: There are more than 40 any constructors. A novice user needs these divided up by purpose so he can quickly find the constructor he needs. The Design notes here: http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/... provide a classification of constructors according to their purpose. This should be part of the tutorial or the tutorial should provide a reference to this. In addition, the tutorial or reference: http://steven_watanabe.users.sourceforge.net/type_erasure/libs/type_erasure/... should categorize the CTORS in different lists under subject headings suggesting the purpose as described in the Design notes. This list categorization would help a novice find the appropriate CTOR. 4. What is your evaluation of the potential usefulness of the library? I've not enough experience in the problem domain to make an informed conclusion. 5. a) Did you try to use the library? Yes. b) With what compiler? ftp://gcc.gnu.org/pub/gcc/snapshots/4.8-20120624/gcc-4.8-20120624.tar.bz2 c) Did you have any problems? Yes, described as follows: [obscure-errors]: The biggest problem was understanding the compiler error messages such as those shown here: http://article.gmane.org/gmane.comp.lib.boost.devel/233026 which gives very little clue about what causes the problem, which was using the any<Concept,PlaceHolder> CTOR taking a binding<Concept> arg where Concept did not include constructible<(PlacHolder(...)>. Based on a this quote: 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. from: http://www.boost.org/doc/libs/1_50_0/libs/concept_check/concept_check.htm maybe using concept_check would help provide clearer error messages. [any<C,T>::any(U&, static_binding<Map>&) bug]: If U is not mpl::at<Map,T>::type, then U will be the contained type, but will be treated as a mpl::at<Map,T>::type. This error was acknowledged here: http://article.gmane.org/gmane.comp.lib.boost.devel/233101 and a possble solution posted here: http://article.gmane.org/gmane.comp.lib.boost.devel/233104 6. How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I studied it in-depth. By that I mean I read most of the docs, and looked closely at some of the source, ran various tests to see what the purpose of some functions were, and created an example program using my own placeholders: struct ph_leafs //placeholder for grammar terminals : placeholder {}; struct ph_roots //placeholder for grammar non-terminals : placeholder {}; struct ph_expr //placeholder for any grammar expresssion : placeholder {}; Which I then used to compile a type-erased grammar of sorts. I also suggested a change to any.hpp as solution to the bug in the any(U&, const static_binding<Map>&) constructor discussed here: http://article.gmane.org/gmane.comp.lib.boost.devel/233101 7. Are you knowledgeable about the problem domain? No, thus more example uses in real world programs would have been helpful.