Tobias Schwinger <tschwinger@neoscientists.org> 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