Re: [boost] [function types][review] (repost with new formatting)

From: Tobias Schwinger (tschwinger_at_[hidden])
Date: 2005-06-13 18:29:01

Hello Dave,

first of all: thank you very, very much for these numerous hints on how to 
improve the documentation - especially for taking the time to rewrite several 
passages. Amazing work!

Mind if I just "merge" some of this into the next revision?

I'll cut down your post for the reply to highlight the points I want to answer 
to -- and add the others to my todo list.

David Abrahams wrote:
> 
> [...] 
> 
> All this stuff goes in namespace boost?  I don't think I'm happy about that.
> 
> 
>>   All names are actually defined inside local namespaces. They are
>>   injected to the boost namespace via a using directive.
> 
> 
> Okay, that's a little better.  It avoids some ADL problems but still
> brings many special-purpose names into the top-level namespace.  Do
> the tag types belong there?  Also "local namespace" isn't a
> well-accepted term.  How about "sub-namespaces of boost?"
> 
> I don't think the synopsis should show them in namespace boost and
> then be amended by a footnote at the bottom that many people might not
> read.  At least put a link to the note in the synopsis code.
> 

I was not too sure about this myself. I did this because most libraries put 
their primary interface into namespace boost.

> 
>>    template<typename F>
>>    struct function_type_arity;
>>
>>    template<typename F>
>>    struct function_type_result;
>>
>>    template<typename F>
>>    struct function_type_class;
>>
>>    template<typename F, typename I>
>>    struct function_type_parameter;
>>
>>    template<typename F, long I>
>>    struct function_type_parameter_c;
>>
>>    template<typename F>
>>    struct function_type_parameters;
> 
> 
> Yowch!  I really hate to see a boost.whatever library with names like
> 
>         whatever_foo
>         whatever_bar
>         whatever_baz
> 
> in it.  Isn't this what namespaces are for?

Okay - if we don't put them in namespace boost, we can also use a different 
naming for these. I like it, especially because the names are currently too 
lengthy for my taste.

>>  is_function_type
>>
>>  Members
>>
>>  value	- 	Static constant of type bool, evaluating to true if T is
> 
> 
> Uhm, the essential member of any metafunction is ::type, and I don't
> see it listed here.  Value doesn't "evaluate to true", it "is true."
> 

is_function_type<Tag,T> is an MPL Integral Constant. The type member is the 
identity of its specialization and not listed explicity because it's part of the 
concept. It won't hurt, I guess.

>>  an element of the set of types specified by Tag. Member function
>>  pointers may be more const-volatile qualified than specified,
> 
> 
> Hmm, have you looked carefully at the use cases for this?  Member
> functions often have counterintuitive is-a relationships.  For
> example, a base class member pointer is-a derived class member
> pointer, but not vice-versa.  It's not obvious to me that any is-a
> relationships should hold here.  What are the use cases?

I'm thinking about removing cv indication from the tags and cv-qualifying the 
class type, instead (see comments in the examples interpreter.hpp : line 99 and 
function_closure.hpp : line 120).

> 
> 
> ---------
> 
> 
>>  Decomposition
>>
>>  Six class templates are provided to extract properties of a function
>>  type, such as arity, parameter-, class (where appropriate)- and
>>  result type.
> 
> 
> "Six class templates are provided to extract properties of a function
> type, such as arity and the types of the result, parameters, and the
> class targets of member functions."
> 
> -------
> 
> 
>>   template<typename T>
>>   struct function_type_arity;
>>
>>  Template parameters
>>
>>  T - The function type to be inspected. T must be a function type as
>>      defined at the beginning of this document
>>      (is_function_type<no_function,T>::value == false).
> 
> 
> You shouldn't repeat that verbose description all over the place.
> 
>   "T - Any function type <link to definition>"
> 
> is better and more concise.
> 
> 
>>  Members
>>
>>  value	- 	Static constant of type size_t, evaluating to the number
>>  of parameters taken by the type of function T describes. The hidden
>>  this-parameter of member functions is never counted. 
> 
> 
> That particular behavior does not match the needs of any use cases
> I've seen.  I always want to treat the hidden this parameter as the
> function's first parameter (actually as a reference to the class, with
> cv-qualification given by that of the member function itself).  What's
> the rationale?

The rationale is that there is no unified call syntax for member function 
pointers and non-member/static function pointers, anyway:

Typically we use a cascade of template specializations that does the actual 
invocation for different function arities.

Only counting "real" parameters here allows us to build such a cascade for 
arities ranging from zero to "OUR_MAX_ARITY_LIMIT" without having to deal with 
the two special cases that there are no nullary member functions and that we 
need to go up to OUR_ARITY_LIMIT+1 for member function invocations if the 
context reference is taken into account.

Think about generating the (hand-written) cascade in

     libs/function_types/example/interpreter.hpp : line 299

with Boost.Preprocessor, for example.

Even if completely binding a function (which I believe is a rather seldom use 
case), we still need a distinction between a member and non-member function 
pointer which won't go away no matter how we put it.

Further we can still take the size of function_type_signature (well, in this 
case the result type is counted as well).

> 
>   "I - An MPL Integral Constant describing the index of the parameter
>   type to be returned. The index of the first parameter is zero."
> 
> What about the hidden "this" parameter?  I think you know the
> semantics I want, but you don't say what you provide.
>

The same argumentation applies here.
And parameters indices should be consistent with the function arity.

Further it makes client code more expressive.

I guess it's your turn here to prove my design is faulty ;-).
If you can convince me, I'll happily change things, of course.

> [...]
> 
> I don't know what you mean by this recommendation.  If I have a
> specialization of function_type_signature, how am I supposed to get
> information about it, or use it?  Am I supposed to pass it to the
> decomposition metafunctions (I don't think so!)?
> 

Use it as an MPL sequence.

I added this recommendation because it led to so much confusion discussing this 
with Jody Hagins (and I probably made things worse).

The recommendation refers to using it as a traits bundle: e.g.

     function_type_signature<T>::arity

instead of

     function_type_arity<T>

which should be preferred.

> 
>>   template<typename Tag, typename Ts>
>>   struct function_type;
> 
> 
> Needs a brief description; maybe all do.  Example: "Returns a new
> function type built from a sequence of types."
> 
> 
>>  Template parameters
>>
>>  Tag	- 	Tag type specifying the kind of function type to be
>>  created. Abstract tag types (ones that describe abstract supersets
>>  of types such as any_function) are not allowed with the exception of
>>  no_function (see below for effect). 	
> 
> 
> "Tag types that describe abstract supersets of types, such as
> any_function, are not allowed with the exception of no_function (see
> below<link>)."
> 
> 
>>  Ts	- 	Model of MPL Forward Sequence of sub types, starting with
> 
> 
> Drop "Model of"
> 
> 
>>  the result type optionally followed by the class type (if Tag
>>  specifies a member function pointer type) 
> 
> 
> That sounds like if Tag specifies a member function pointer you have
> the option to represent the class type.  Just drop "optionally" and
> remove the parens.
> 
> So here you are using the form I like, where class types are treated
> just like any other (should be a reference, though).  Why not do this
> uniformly.
>

See above.

>>  followed by the parameter
>>  types, if any. If Ts is not a sequence (mpl::is_sequence<T>::value
>>  == false), a specialization of function_type_signature for this type
>>  is used. 
> 
> 
> This is unintelligible.  You already said Ts is required to be a
> sequence.  Now you're taking it back? A specialization of

I remember being rather unhappy about this writing it...

> function_type_signature for *which* type is used?  Used how?
>

... it needs to be rewritten.

> 
>>  This may fail if function_type_signature is an incomplete
>>  template due to forward-declaration without definition.
> 
> 
> Why would that ever be the case?  Shouldn't your library take care of
> it?
> 

Both 'function_type_signature' (which is the backend for all higher-level 
inspection components) and function_type need quite a bit of code (and when used 
in preprocessing mode a significant ammount of compile time).

That's why I decided to not let one depend upon the other, but to make them plug 
into each other when both are defined.

>>  . If Tag is no_function,
>>  type is identical to Ts if Ts is an MPL Sequence other than a
>>  specialization of function_type_signature, in which case an type is
>>  an mpl::vector (this is primarily for internal use of the library
>>  but, since it may be useful in some situations, there is no reason
>>  to hide it from the user).
> 
> 
> Oh yes there is!!  I can't for the life of me understand these
> semantics.  Guideline: anything whose specification is too complicated
> to explain easily probably needs to be redesigned.

It used to be a bit more understandable before I removed a rather strange 
feature to grab the signature from a class template's parameter list. I will 
just remove this as well.

> 
> Also, special-case "if" clauses, especially those that test whether
> something matches a concept (like "is_sequence") tend to destroy
> genericity.
>

Would you mind explaining this in some more detail?

> ----
> 
> 
>>  Portability
>>
>>  Currently this library does not contain any workarounds for broken
>>  compilers. It was successfully tested with Microsoft Visual C++
>>  Versions 7.1 to 8.0, Intel Linux Versions 7.0 to 8.1 and GCC Version
>>  3.2 to 4.0. The library is expected to work with compiler using the
>>  Edison Design Group frontend other than Intel, although currently
>>  there is no proof for this.
> 
> 
> Okay, I appreciate the desire to avoid broken compiler workarounds.

Okay. The desire to avoid broken compiler workarounds applies especially to the 
review version.

I had hoped the review to answer how much portability is required for proper 
Boost integration. I definitely plan on extending the list of supported compilers.

> However, many of the libraries in Boost that could use this library do
> support broken compilers, and therefore -- I think -- are not using
> idioms that can readily leverage metafunctions for this purpose.  The
> true measure of whether this library should be accepted, IMO, is in
> whether it can be used to eliminate large amounts of code from other
> libs.  Can it?  The quickest way to a proof-of-concept is to apply it.
> 

Good idea!

> Incidentally, I think
> http://lists.boost.org/MailArchives/boost/msg81758.php makes it
> possible to implement almost any trait we previously thought was
> impossible in pre-7.1 Visual C++.

If the folks at Borland don't fix their compiler they should at least add this 
as a new bug...

> I'm not ready to accept this library, and I'm not ready to reject it
> either.  It has the potential to be extraordinarily useful, but that's
> unproven as far as I'm concerned.  I have some substantial complaints
> with the interface, and I have some serious problems iwth the docs as
> you can see (not all of those remarks are trivial grammatical edits!)

Summary:

- proof it by making some boost libraries use it
- change naming
- change docs

I hope (but am not entirely sure) it's all doable within the review period.
Any hints on a preferred prioritization?

Regards,

Tobias