AMDG On 08/02/2012 12:29 PM, Larry Evans wrote:
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.
I added a static assert.
<snip>
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.
I think something like this is reasonable. I have to consider whether this is best approach, though. I don't consider this high priority, however, since the copy constructor should work most of the time. The STL got by just fine for years without emplace.
[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:
The solution is simple. Don't use the source. Use the documentation page for any, which only shows 9 distinct constructors.
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.
Why?
Of course, I'm probably missing something, and if so, explaining why the above would not work in the Design notes would be helpful.
I think it should be obvious what's missing: a) any(const any&); The copy constructor is not a template. b) template<class U> any(const U&); This is the most commonly used constructor besides the copy constructor. It would be very inconvenient if it didn't exist. c) any(); The default constructor creates an empty any. d) A default argument can't refer to a prior argument. Your fourth constructor has to be two separate constructors.
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.
The tests only cover the public interface. I'm not going to add specific tests of the implementation details.
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.
Huh? As the documentation you quoted says, rebind_any returns a specialization of /any/. rebind_any<any<C, S>, T>::type is any<C, T>.
<snip>
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.
That depends on the context. boost::type_erasure::call "unwraps" these any's.
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*** */ <snip>
I find this code almost completely unreadable.
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.
Already fixed. In Christ, Steven Watanabe