Hi Richard, Richard Smith wrote:
Throughout these comments, I am assuming the namespace alias
namespace bft = boost::function_types;
1. Namespace
The public interface to this library is in the boost::function_types namespace. As a propsective user of the library, it feels very much like an extension to the type_traits library, and that leaves me wondering why the library isn't in the main boost namespace with the other type_traits functionality.
Well, we can probably pull some of the templates down to boost (see below) in case the library gets accepted into Boost.
2. Use of Tags
After a second glance, it's clear that one reason for having separate namespaces is to avoid name collisions -- we already have a boost::is_function, for example. And this leaves me wondering how boost::is_function differs from bft::is_function as the names fail to convey the distinction. So far as I can see, the additional Tag template parameter is the only difference, and only three of the possible property tags are meaningful: variadic, non_variadic and default_cc.
No. In fact is_function is a candidate to be put in namespace boost. The tag parameter has a default, so boost::is_function and bft::is_function are compatible.
The majority of the time I can't imagine myself making much use of any of these tags. Are they really worth while? The 'Rationale' section of the documentation addresses this and considers some alternatives:
<snip quote>
What about the third possibility: leave it up to the client to sythesise these more complicated examples. It will involve adding a few more templates: notably is_variadic and is_default_cc. Then, instead of writing
bft::is_function< T, bft::variadic >::value
you would write
boost:is_function<T>::value && boost::is_variadic<T>::value
... which is not significantly more verbose. I think this would be my prefered interface as it would avoid the duplication of functionality between the type_traits library (and the potential for confusion that this would cause).
I think, I don't like it. It's too irregular for my taste.
3. Extensibility
The internal bitmask implementation underlying the property tags does not appear to allow extension by a user.
Hell - don't mess with the bitmask - it's a library internal ;-) !
For example, there is a default_cc tag; suppose I want to add support for querying the some other calling convention.
You can.
Can I write a fastcall_cc tag?
Even better: It's written for you.
Scanning through the implementation it would appear not -- certainly there's no documentation on how to do so.
All you have to do is set the macros BOOST_FT_CC_NAMES and BOOST_FT_CC_<name> appropriately (see Reference->Macros).
If a tag-like syntax is considered desireable, wouldn't something like
template < typename T, template <typename> class UnaryTypeTrait > struct is_function : integral_constant< bool, is_function<T>::value && UnaryTypeTrait<T>::value > {};
be more extensible? Or does this have to work with compilers that can't handle template template parameters?
No. But it's supposed to work with MPL-Lambda Expression, which do not allow template template parameters.
4. Naming
It seems unlikely that the default_cc tag will be particularly heavily used. Given this, why not give it a more meaningful name: default_calling_convention perhaps?
For the default configuration all calling convention tags currently have a _cc suffix. But we might as well s/_cc/_calling_convention/g.
5. Extern "C" linkage
<snip>
(mostly because you can't put extern "C" within a template or atemplate within an extern "C" block).
Exactly.
6. What is a 'callable builtin'?
So far as I can see, the C++ Standard does not define 'callable', so I asume that the pertinent definition is the one in TR1, 3.1/6:
| A /callable type/ is a pointer to function, a pointer to | member function, a pointer to member data, or a class | type whose objects can appear immediately to the left of | a function call operator.
By this definition, function types are not callable, nor are references to functions, bizarre as this may seem.
<snip>
Though I'm not wholly convinced with TR1's definition of 'callable', this is as close as it gets to a standard definition, and it would seem wise to make pointers to member data return true.
I also think 'builtin' needs documenting even if only superficially; to me 'builtin' is a synonym to 'fundamental', though there are no callable fundamental types.
Obviously these terms do not work well... We need a definition or we have to find a better way of naming the beast.
7. What is a 'callable scalar'?
The term 'scalar' is defined in 3.9/10:
| Arithmentic types, enumeration types, pointer types, and | pointer to member types, and cv-qualified versions of | these types are collectively called scalar types.
Note that references, whether to functions or otherwise, are *not* scalars. The is_callable_scalar metafunction returns true for references to functions. This is wrong.
Doh!
Also, why is is_callable_scalar useful? I'm not sure I can think of a use case; and if I could, I imagine it would be rare enough that it wouldn't be much of an inconvenience to sythesise it from is_function and is_callable_builtin.
Good point. Probably we can just throw it out.
8. 'Tag Types' documentation
In several places, the 'Tag Types' documentation incorrectly gives types two leading underscores.
Where?!
9. Extending result_type
Perhaps this is beyond the intended scope of the library,
Yes.
but I think it would be very useful to extend the domain of this metafunction to include class types with a result_type member type.
No, that's what boost::result_of is for. FunctionTypes is only about the type system. One of its examples is a reimplementation of boost::result_of, in fact. <snip>
10. Syntactic sugar around parameter_types
Whilst being able to leverage the full power of the MPL with parameter_types is definitely desireable, it can also faze those less familiar with metaprogramming.
I would like to suggest a helper metafunction that extracts a single parameter type:
template < typename F, std::size_t N, class ClassTransform = add_reference<_> > struct function_parameter { typedef typedef mpl::at< function_parameters<F, ClassTransform>, N >::type type; };
(I'd suggest base-zero numbering as given here, though this might cause confusion in conjunction with the first_argument_type, second_argument_type typedefs in std::binary_function.)
Good practice is to typedef the sequence and use mpl::at_c on the new name. It also leads to more readable code, IMO. Further, I think it's a good idea to encourage people to use the MPL.
Oh, and I'd also like to suggest a quick comment in the documentation saying what the '_' is that is given as a template argument to add_reference -- a quick
using mpl::placeholders::_;
in the synopsis would probably be sufficient.
I'll add that.
11. Member function arities
The documentation to function_arity states that the 'this' pointer is included -- which is what most people would expect. It's also what the implementation does. However in the 'Use Cases' page of documentation, a R (C::*)() function is listed in the first code fragment under the comment '0 parameters'. Although not explicitly contradictory (parameters and arity don't *necessarily* mean the same thing), I think this is potentially confusing.
I'm afraid that making the same point while addressing this issue could be more confusing. Any suggestions?
12. Use cases documentation
To be honest, I find this page of the documentation dreadful, which is shame as the rest is quite reasonable. It discusses various scenarios that could benefit from the application of this library, but utterly fails to suggest *how* they would benefit from the library.
On the whole page, only one of the library's components is actually mentioned -- the function_pointer metafunction for synthesising function types. But the code fragment is far too incomplete to be of any real use. It's also one of the more complicated examples as it requires using the MPL. By all means include such a example, but not as the only one!
Did you follow the links? There's a lot of inline documentation in the files. But maybe I don't get your point. Further, as mentioned above, I'm all against MPL-fear-mongering.
13. Why is bft::components useful?
For that matter, what *exactly* does it do?
It gives a one-type respresentation of all properties. And IIRC there is at least one example that shows it in action.
First the documentation says
| components<T,ClassTransform> | MPL - Front / Back Extensible Random Access Sequence | of all component types and property tag
then it says
| Extracts all properties of a callable builtin type, that | is the result type, followed by the parameter types | (including the type of this for member function | pointers).
The former is ambiguous -- it doesn't say what it means by 'component types'; > and the latter contradicts it by not mentioning the property tag.
It also seems not to mention what it actually means by *the* property tag. A function can be variadic or non_variadic; it be default_cc; it may have cv-qualifiers, which in the case of a const volatile function needs to tags to express it. Are these concatenated using bft::tag or appended one-by-one to the end of the MPL sequence?
The expression models two concepts.
But more importantly, why do we need this metafunction? It does nothing that separate invokations of result_type, parameter_types and the various is_* metafunctions can't achieve. And I would *rather* see separate invokations of the these -- it would make for much more readble code.
Oh, and the name isn't very descriptive.
Well, read it like function_type's::components . I've seen names worse than that...
14. Type sythesis
... And now I see what bft::components is supposed to achieve.
But I still don't find the idea of merging the result type, parameter types and arbitrary other information together into a single type list at all intuitive.
What's wrong with:
template < typename Result, class ParameterSeq, class PropertiesTag = null_tag > struct function_type;
Much more readable, in my opinion.
Maybe. But I'm not sure.
Having said that, I think these metafunctions are an important piece of functionality. It's merely their interface I'm concerned about.
I hope these points have been useful -- and sorry for such a long post if they haven't!
Nothing to be sorry about. Thank you for your review! Regards, Tobias