|
|
Boost : |
Subject: Re: [boost] [type_erasure] Review started (July 18-27, 2012)
From: Larry Evans (cppljevans_at_[hidden])
Date: 2012-08-07 16:04:57
On 08/03/12 13:39, Steven Watanabe wrote:
> 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.
Then I withdraw that objection :)
>
>> <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/doc/html/boost/type_erasure/any_Concept__T___id2445315.html#id2445396-bb
>>
>> 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]:
>>
[snip]
>> 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?
>
Someone trying to understand the source would have less to understand
if all the other CTOR's were simply defined in terms of these, more
basic CTORS, as shown in the refactoring below. This defining of one
CTOR in terms of other, more basic CTOR's, makes the source easier to
understand, a viewpoint expressed by:
Refactoring is a good thing because complex expressions are
typically built from simpler, more grokable components.
from:
http://c2.com/cgi/wiki?WhatIsRefactoring
Also, putting a description of this in the docs would make
understanding how to use the library easier. The mpl docs, which I've
found very usable, do something similar where they describe one
metafunction in terms of other, more basic metafunctions. For
example:
http://www.boost.org/doc/libs/1_50_0/libs/mpl/doc/refmanual/fold.html
contains:
Semantics:
Equivalent to
typedef iter_fold<
s
, state
, apply_wrap2< lambda<op>::type, _1, deref<_2> >
>::type t;
>> 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.
Of course you're right that the 4 basic CTOR's don't include the one's
you mention. I should have been more explicit about the purpose of
those CTOR's. I was trying to say that all the CTORs could be defined
in terms of just these 4 basic CTOR's, which is a just a refactoring.
http://en.wikipedia.org/wiki/Code_refactoring
I sort of hinted at this refactoring purpose, with the following from
my review:
(except for maybe requiring more typing and maybe more
constructor calls)?
I realize now that hint was not obvious. Sorry. The refactoring
below makes this explicit.
In addition, I really should have actually tried to refactor all the
CTOR's just using these basic, let's call them any_core, CTOR's.
These basic CTOR's, after renaming, would then be as shown next. (The
CTOR's are prefixed with "labels", such as //[c1], to make referencing
them easier later.):
//[c1]:
template
< class... U
>
any_core
( static_binding<Map>const&
, U&&...
);//kind=binding constructor
//[c2]
template
< class... U
>
any_core
( binding<Concept>const&
, U&&...
);//kind=dispatching constructor
//[c3]
template
< class Concept2
, class Tag2
>
any_core
( any_core<Concept2,Tag2>U&&
, static_binding<Map>const&
);//kind=converting constructor
//[c4]
template
< class Concept2
, class Tag2
>
any_core
( any_core<Concept2,Tag2>U&& u
, binding<Concept2>const&b
);//kind=converting constructor
Basically, this refactoring would:
1) Use any_core<,> instead of compute_bases<...> as the supertype of
the refactored any.
2) Use the current_compute_base<...> as the supertype of
any_core<,>.
Then the missing CTOR's, which you enumerated in your a)...d) list
above, would use any_core as shown in this refactored 1. ... 9. list
The 1. ... 9. list corresponds to the nine CTOR's listed in the doc:
and this was done (instead of using the a)...d) CTOR list in your
reply ) to assure I don't miss anything this time.
1. any();
any()
: any_core<Concept,T>
( make_binding
< mpl::map
<
>
>()
)//[c1]
{}
2. template<typename U> explicit any(const U & data);
template<typename U>
explicit any(const U & data)
: any_core<Concept,T>
( make_binding
< mpl::map
< mpl::pair<T,U>
>
>()
, data
)//[c1]
{}
3. template<typename U, typename Map>
any(const U & data, const static_binding< Map > & binding);
template<typename U, typename Map>
any(const U & data, const static_binding< Map > & binding)
: any_core<Concept,T>
( binding
, data
)//[c1]
{}
4. any(const any & other);
any(const any & other)
: any_core<Concept,T>
( binding_of(other)
, other
)//[c2]
{}
5. template<typename Concept2, typename Tag2>
any(const any< Concept2, Tag2 > & other);
any(const any< Concept2, Tag2 > & other)
: any_core<Concept,T>
( binding_of(other)
, other
)//[c2]
{}
6. template<typename Concept2, typename Tag2, typename Map>
any(const any< Concept2, Tag2 > & other,
const static_binding< Map > & binding);
any
( const any< Concept2, Tag2 > & other
, const static_binding< Map > & binding
)
: any_core<Concept,T>
( other
, binding
)//[c3]
{}
7. template<typename Concept2, typename Tag2>
any(const any< Concept2, Tag2 > & other, const binding< Concept >
& binding);
any
( const any< Concept2, Tag2 > & other
, const binding< Concept > & binding
)
: any_core<Concept,T>
( other
, binding
)
{}//[c4]
8. template<class... U> explicit any(U &&... arg);
explicit any(U &&... arg)
: any_core<Concept,T>
( _boost_type_erasure_extract_table
(
decltype(this->_boost_type_erasure_deduce_constructor(arg...))
, arg...
)//This expression has type, binding<Concept>const&
, arg...
)//[c2]
{}
9. template<class... U>
explicit any(const binding< Concept > & binding, U &&... arg);
explicit any(const binding< Concept > & binding, U &&... arg);
: any_core<Concept,T>
( binding
, arg...
)//[c2]
{}
NOTE: this has not been coded to actually test to see if it works;
hence, the above may be missing something and not even compile.
The 'core' components, any_core, closely reflect the 3 kinds of
constructors you mention in
design.html#boost_typeerasure.design.constructors; hence, I hope the
advantage of such refactoring is more obvious to you.
Another viewpoint is that the 9 any's are "shorthand" for more
expressions and or statements using just any_core. Since you
described the meaning of tuple using somewhat analogous "shorthand"
here:
http://article.gmane.org/gmane.comp.lib.boost.devel/232973
I'm hoping that this other viewpoint further convinces you.
[snip]
>>
>> 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/doc/html/boost/type_erasure/rebind_any.html
>>
>> 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?
Initially, I thought:
returns any type corresponding to a placeholder.
meant rebind_any returns the underlying type corresponding
to the placeholder. IOW, the type:
mpl::at<Map,T>::type
where Map was the template argument to the any CTOR:
template<typename U, typename Map>
any(U & arg, const static_binding< Map > & binding);
However, since rebind_any doesn't use any instance of a
type_erasure::any, there's know way it know what Map is. I had to
figure out what the doc really meant, and the one that made sense
was:
returns any<C,T> where the Any argument to rebind_any
is any<C,S> for some placeholder, S.
> As the documentation you quoted says, rebind_any returns a
> specialization of /any/. rebind_any<any<C, S>, T>::type
> is any<C,T>.
AFAICT, the documentation quoted was from:
which made no mention of any<C,T>. There was a mention of any<C,T> here:
http://article.gmane.org/gmane.comp.lib.boost.devel/233015
which contained:
Hmmm, do you mean the return type(i.e. rebind_any<any<C,S>,T>::type)
is any<C,T>?
which is an earlier version of what I'm suggesting above as a
replacement for the existing:
returns any type corresponding to a placeholder.
Since you referred to that quoted document, I assume you're agreeing
that the
rebind_any documentation could be improved using something like the
suggestion in that quoted document. Is that right?
>
>> <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.
The part of my reply that was <snip>'ed included:
----snipped part----
[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/doc/html/boost/type_erasure/any.html#id2445089-bb
----snipped part----
So, obviously, the relevant context was the documentation for:
explicit any(const binding< Concept > & binding, U &&... arg);
( and just as obviously, my label:
[vague:any<C,T>::any(U&, binding<C>const&)]
should have been:
[vatgue:any(const binding< Concept > & binding, U &&... arg)]
Sorry.
)
And in that context, the above conclusion is right?
In addition, the 'depends on context' is the crux of the problem. If
there had been documentation on what a placeholder in a particular
context, then there would have been no problem. And just to be sure,
some accompanying examples to cement the meaning would be most
helpful.
> boost::type_erasure::call "unwraps"
> these any's.
Yes, I figured as much; although I certainly don't know how.
>
[snip]
>> 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.
The attached file:
any_ctor_reformatted_kinds.cpp
is a reformatting of the code in a form as close as I could make to
the same way you format your code while still minimizing the code
enclosed by the #if...#endif's.
Hopefully, the attached is more readable.
>
>> 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/doc/html/boost/type_erasure/tuple.html
>>
>> makes no mention of how the binding produced by this call are
>> generated.
>>
>
> Already fixed.
>
I just looked at the file and I don't see a simple and
explicit description of how the bindings are produced.
Currently(August 5, 2012), it contains:
---cut here---
Description
tuple is a Boost.Fusion Random Access Sequence containing
anys. Concept is interpreted in the same way as for any. The remaining
arguments must be (possibly const and/or reference qualified)
placeholders.
tuple public construct/copy/destruct
1. template<class... U> explicit tuple(U &&... args);
Constructs a tuple. Each element of args will be used to
initialize the corresponding member.
---cut here---
Which is still a vague explanation. The one posted in my review is at
least a
little more explicit about how the binding are produced.
-regards,
Larry
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk