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

From: David Abrahams (dave_at_[hidden])
Date: 2005-06-13 20:57:48

Tobias Schwinger <tschwinger_at_[hidden]> writes:

> 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!

My pleasure.

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

Go for it.

>> 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.

Great.

>>>  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. 

You don't say that anywhere.

> 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.

It's necessary.  Until you say it is an Integral Constant, there's no
relationship required between ::type and ::value, so even if it's a
metafunction you had left its result undefined.  More importantly, few
readers are likely to be intimately familiar with the MPL concepts,
and most will miss the connection.

>>>  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).

Not gonna look at the source, sorry.  No time.

>> 
>>>  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:

Yes, but I typically build something that *does* have a common call
syntax when wrapping member functions, i.e. I build a function object.
I would prefer if the metafunction would reflect its signature rather
than forcing me to assemble it from this nonuniform blob.

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

Whaa?

I don't have any "cascading" template specializations in Boost.Python
AFAIK.  I don't think Boost.Bind does either.

> 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.

Okay, I've dealt with that issue.  But it's a (really) minor one.
Generating these things with the preprocessor should usually be done
with vertical repetition (you'll get lousy compile times and no
debuggability otherwise), which makes that sort of iteration bounds
adjustment trivial.

> Think about generating the (hand-written) cascade in
>
>      libs/function_types/example/interpreter.hpp : line 299
>
> with Boost.Preprocessor, for example.

Sorry, no time to look at the code.  But anyway, I've been there.
It's easy.

> 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.

I agree with that but don't see the relevance.

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

?? You mean sizeof()?  or mpl::size<>?

>>   "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.

I still disagree with it :)
But regardless, **you need to document the semantics you do provide**.

> And parameters indices should be consistent with the function arity.

I agree with that, but don't see the relevance.  In my world, the arity of 

  int (foo::*)(int)

is 2.

> Further it makes client code more expressive.

On what do you base that claim?

> I guess it's your turn here to prove my design is faulty ;-).

It's not so much faulty as needlessly irregular.  I believe that will
be an inconvenience in some real applications, and at the very least
will cost more template instantiations than necessary.

> If you can convince me, I'll happily change things, of course.

How'd I do?

>> 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).

It may just be in the phrasing.  There's too much detail missing.

> 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.

But the latter doesn't look like a "use" of function_type_signature at
all! Anyway, don't clarify your meaning for mehere; propose a
documentation fix.  Just look very carefully at the words you used
(like "use," "primary interface," "white box," etc.), and consider how
they might be (mis)interpreted by someone who doesn't already know
what you're talking about.

>>>  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.

Still waiting for a satisfactory answer.  Seems to me that your
library only gets easier to use (and learn!) if it traffics in one
uniform structure.

>>>  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.

Well, I suggest you say something that the average dummy is more
likely to understand, like, "you better #include <whatever> or this
won't compile."

>>>  . 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 find it hard to believe that it was easier to understand when there
was an *additional* feature in its behavior!

> I will just remove this as well.

That sounds like it goes in the right direction.

>> 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?

Take boost::variant, which (at one time -- still?) would accept up to
N arguments describing the held types, *OR* an MPL sequence of the
types.  The problem happens when your generic code wants to create a
variant of one element that happens to be an MPL sequence type.  Now
you need a special case that sticks that sequence in another
sequence.  Well, in that case the sequence interface is the only one
of any use to you, isn't it?

The point is that switching on a type's properties often introduces
nonuniformity of semantics, which hurts genericity.  I didn't analyze
your case to see if it was a problem here in particular.

>> 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...

:o)

>> 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

- Improve uniformity, if I can get it.  I would probably accept the
  lib without that change, but I fear I will grumble every time I use
  it.

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

  1. Proof it
  2. Post updated docs that include the proposed naming changes
  3. Change the code.

But I'm not too particular about it.  I think the proof should come
first because I'd happily accept assurances in place of steps 2 and 3
happening before the review period ends.

-- 
Dave Abrahams
Boost Consulting
www.boost-consulting.com