|
|
Boost Users : |
Subject: Re: [Boost-users] [boost] [type_erasure] Review started (July 18-27, 2012)
From: Larry Evans (cppljevans_at_[hidden])
Date: 2012-08-02 15:29:44
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:
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:
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/detail/normalize.hpp
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/test/
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:
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:
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:
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:
contains:
Requires:
Concept must contain a matching instance of constructible.
with no definition of "matching". There is something about
"relaxed_match" documented here:
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:
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/doc/html/boost_typeerasure/design.html#boost_typeerasure.design.constructors
#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:
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:
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:
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.
Boost-users list run by williamkempf at hotmail.com, kalb at libertysoft.com, bjorn.karlsson at readsoft.com, gregod at cs.rpi.edu, wekempf at cox.net