I've spent some time this afternoon reviewing the contents of the function type library as its functionality is something I feel is currently missing from Boost and would like to see added. Although most of my comments are negative, I like this library, and would like to see it in Boost. I would prefer to see some modifications made first, but even in the absense of these, I'd still like to see it added. There were several different links posted; I'm assuming that <http://tinyurl.com/qaf5f> is the correct one. 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. 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. 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: | The first one is just using more templates so every | property has to be asked for explicitly. This approach | results in more complicated client code if more than one | propery has to be checked and in a exponentially larger | library interface. | | The second alternative is having the client pass in bit | patterns via non-type template parameters. The logic has | to be performed by the client and there are much more | error conditions. Further, class templates with non-type | template parameters do not work within MPL lambda | expressions and can cause problems with older compilers. 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). 3. Extensibility The internal bitmask implementation underlying the property tags does not appear to allow extension by a user. For example, there is a default_cc tag; suppose I want to add support for querying the some other calling convention. Can I write a fastcall_cc tag? Scanning through the implementation it would appear not -- certainly there's no documentation on how to do so. 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? 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? 5. Extern "C" linkage What should this print? extern "C" { typedef int fn(); } int main() { std::cout << bfs::is_function<fn, bfs::default_cc>::value << std::endl; } As function types differing only by language linkage are distinct types (7.4/1) (even though many compilers don't respect this), they can, in principle, have differing calling conventions, in which case the extern "C" calling convention is not default. This may be a non-issue as I don't know whether any of the compilers that Boost targets actually take advantage of this, and even if they do, I don't even know whether it is possible to detect "C" language linkage with template metaprogramming (mostly because you can't put extern "C" within a template or atemplate within an extern "C" block). 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. I can't find any definition of 'builtin' in either the Standard or TR1, though it is occasionally used in the Standard to refer to integral or arithmetic types -- clearly this isn't what's meant here, though! It would appear that in this context 'callable builtin' is supposed to include function types, pointers or references to function types and pointers to member functions. It does not include classes with an overloaded operator() (unsurpisingly -- it would be a very strange definition of 'builtin' that included them); nor does it include pointers to member data, even though they are 'callable' per TR1 and are seemingly as 'builtin' as pointers to member functions. 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. 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. 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. 8. 'Tag Types' documentation In several places, the 'Tag Types' documentation incorrectly gives types two leading underscores. 9. Extending result_type Perhaps this is beyond the intended scope of the library, 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. The rationale of this is that in many ways bft::result_type is a simplified version of boost::result_of -- simplified so that the types of the invokation arguments need not be known. In other words, if both of these compile result_of< Fn( T1, T2, ... ) >::type result_type< Fn >::type compile, they will refer to the same type. (This is also true of the current implementation without my suggested extention.) 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.) 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. 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. 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! 13. Why is bft::components useful? For that matter, what *exactly* does it do? 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? 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. 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. 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! Cheers Richard Smith