Just starting from the beginning: This library contains .inl files. In my opinion these should be .hpp files, but if you insist on a different extension, please make them .ipp. Nico Josuttis made the argument for .cpp/.hpp in the early days of Boost and it remains a good one: it's easier to find all the source files with various tools, and you know which ones contain C++ source. ,---- | Use fewer words | | - E.B. White `---- Overview This library provides a metaprogramming facility... I think you should drop the word “metaprogramming” here. Lots of people who don't think of themselves as doing metaprogramming might be interested in this library and could easily be scared off. Also, it adds nothing to the essential meaning of the sentence For the purpose of this documentation, these types are collectively referred to as function types (this differs from the standard definition and redefines the term from a programmer's perspective to refer to the most common types that involve functions, however, classes with an overloaded parentheses operator are not considered a function type, here). Put the parenthesized material in a footnote. It's too long to be part of that sentence and anyway it's too much detail for 99% of readers, who won't care about the standard's definition. The classes introduced by this library shall conform to the concepts of the Boost Metaprogramming library (MPL). This library introduces templates, not classes. Drop the word “shall.” It's unnatural (“shall” is normally used in declarations of what will happen in the future) and adds nothing. doesn't really conform to the way the C++ standard uses it, which is a statement of requirements on what implementations of the standard must do -- unless of course you think you're writing a standard that other people will implement. Also, you don't say which concepts. I would just say, “This library is designed to work well with the Boost Metaprogramming library (MPL),” or say earlier on “This is a library of metafunctions <link to MPL concept definition>,” if that's accurate. Yes, I know this somewhat contradicts my admonition to drop “metaprogramming.” The Function Types library enables the user to: * test an arbitrary type for being a function type of specified kind, “test whether a type is a specific kind of function type” * inspect properties of function types, such as function arity, result parameter types, etc. Drop the 1st comma * view and modify sub types of an encapsulated function type with MPL Sequence operations, and No space in “subtypes” (sub is not a word on its own -- repeated throughout the document). What is an “ecapsulated function type?” * synthesize function types. A tiny bit more detail about what this means should go here. I am guessing it means function_ptr<R, A1, A2>::type ==> R(*)(A1,A2) That would be “synthesize function types from argument and return types.” Motivation Applying metaprogramming to increase the flexibility of software design often involves the inspection and manipulation of function types. A very common application is the automatic generation of callback facilities which occurs frequently in Boost libraries, such as Bind, Lambda, Phoenix and Function. This is a very weak opening. A person reading the Motivation section wants to know, “what would I do with this library?” I suggest something like: Generic libraries that accept callable arguments are common in C++: STL, Boost.Lambda, Boost.Function, and Boost.Python are just a few examples. Analysis and manipulation of built-in function types seems to appear in each one. The above is by no means perfect, and it took me 5 minutes to write. It can be hard to follow Mr. White's advice ;-) Other (often related) applications, include creating interface descriptions from function prototypes (extracting meta information about an interface). That's so redundant it's not worth saying. All the periods in this paragraph should be commas. concept checking (e.g. find out if two functions model a setter/getter pair). and systematic overload selection (creating a function pointer to e.g. std::sin requires a cast to either select the overload for float, double or long double). Other applications include - concept checking [Show brief example asserting that two functions model a setter/getter pair] - systematic overload selection [Show brief example of how creating a function pointer to .g. std::sin requires a cast to either select the overload for float, double or long double] Currently Boost libraries redundantly use template partial specialization or overloading cascades to deal with function types. This has the following disadvantages: Can't use “this” without an antecedent. “This approach has the following disadvantages:” * It is tedious to write when the library is implemented, It is tedious to implement. * most libraries fail to support functions with non-default calling convention or variadic parameter lists * proper support for the former point results in redundant work Drop "point" for the authors, redundant per-library configuration settings Need a comma right here. and redundant code being processed if these libraries are used in the same program. This library gives developers a powerful tool at hand to avoid the above problems. Drop "at hand." But I'd probably drop the whole sentence. It covers the functionality of the function_traits class (introduced by the Boost Type Traits library), which has been agreed to be unsuitable for template metaprogramming in most situations, due to the fact that its interface encodes the index to function paramter types in identifier names and the inability to work with types other than plain non-member/static functions, which, from today's perspective, makes function_traits a weak spot of the otherwise excellent library. This sentence is too long! I suggest: This library provides all the functionality of the function_traits <link> class template from the Type Traits library in a form that's more suitable for template metaprogramming. If you like, add a link to a detailed explanation of the reasons that function_traits is unsuitable, in a setting where you have the space to write something comprehensible to non-gurus. There's no room to say anything useful about that in the introductory section of your docs. Other Type Traits classes covered as special cases by this library's is_function_type class are is_function and is_member_function_pointer. "Also, is_function and is_member_function_pointer are covered as special cases by this library's is_function_type class template." --------------- Interface Some simple examples are missing right here. In fact, I've gone well past a whole page of reading with no examples. I don't think we should introduce any new libraries without a simple example in the 1st page. The rest of the doc has some examples, but only as linked documents. I haven't clicked through, but I imagine they're complete, compiling programs. That's too much information, too distantly located, to be useful to the person trying to get a feel for the library. Note also that many people will be reluctant to click through those because it will launch their IDE instead of displaying in their browser. --------------- Synopsis: 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. 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? The [...see Tag Types...] link just takes me to the top of the doc. ---------------- Headers Header file names generally match the name of what their inclusion causes to be defined with the exception of function_type_parameter.hpp which also contains the class function_type_parameter_c. "With one exception, each component is defined in a header file whose name is the same as that of the component, with an .hpp suffix. For example, before using is_function_type, your functions should first contain: #include <boost/function_types/is_function_type.hpp The exception to this rule is function_type_parameter_c, which is defined in boost/function_types/function_type_parameter.hpp." The tag types are defined as soon as the first header containing a class that depends on them is included. The headers don't contain classes. At this point, I have no clue what these dependencies are and can't understand what this statement means. All I can guess is that I don't ever have to worry about specific #includes for the tag types. If that's the case, just tell me so :) ------ Tag types As stated at the beginning of this document, there can be different kinds of function types: # non-member/static function, # pointer to non-member/static function, # reference to non-member/static function, # member function pointer, # member function pointers that are const-volatile qualified, # each of the above with a parameter list ending with an ellipsis operator, and # each of the above for every calling convention defined by the C++ implementation in use. The Function Types library uses tag types to describe differnt kinds of function types, and arbitrary supersets of these and their inverse. [Show an example of a simple tag type in use] You can't represent "arbitrary supersets;" you only represent the supersets that you've chosen to support. "Each kind of function type has a corresponding tag type: * non-member/static functions: plain_function * pointers to non-member/static functions: function_pointer * references to non-member/static functions: function_reference ...etc... There are also tags to represent groups of function types: * Any function type at all: any_function ...etc..." The document doesn't say anywhere what no_function is. It has no relevance to the user, how these tags are implemented Drop the comma. except that they exist and that they are types that can be passed as template type arguments. -------- Classification I always learned that it is bad form to have a section with only one subsection. You have several of these. I would list all of the metafunctions except for the decomposition metafunctions at the top level and have a section called "Decomposition Metafunctions". Except there seems to be something missing alongside is_function_type: I want a metafunction that returns the tag corresponding to a given function type's kind. ---- 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." 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? Need "and" right here. top-level const-volatile qualification of pointers is always ignored. Unless explicitly specified otherwise by the Tag, both "Unless the tag explicitly indicates otherwise, ..." variadic and non-variadic function types of any calling conventions "convention" are matched. ... - See the MPL Integral Constant concept. What?? What kind of member is "..."?? This is unacceptably vague. Header <boost/function_types/is_function_type.hpp> Member function pointers may be more const-volatile qualified than specified --------- 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? If T describes a variadic function prototype (e.g. printf) the ellipsis parameter is not counted. -------- template<typename T, typename I> struct function_type_parameter; Template parameters ... I - The index to select the parameter type to be returned (MPL Integral Constant). Valid indices start with zero (for the first parameter). Putting "MPL Integral Constant" in parens has no agreed-upon meaning and seems arbitrary and needlessly terse. "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. -------- Encapsulation While the decomposition part of the interface provides a fine Need a hyphen here. grained set of traits classes to find out distinct aspects of a ^^^^^^^^ "discover," or "query" function type, specializations of the class template function_type_signature in contrast represent a function type in order to work with it as a whole. Therefore function_type_signature can be used with MPL Sequence operations to work with the representee type's sub types. ^^^^^^^^^^^ "represented" This begins to sound like function_type_signature<T> is an MPL sequence. I would just drop the sentence. Oh, wait, after reading the whole description I now believe it is a Random Access Sequence... but you never tell me what the sequence contains!! This is a serious omission. Other properties, such as arity, kind of function type and the representee type, are contained as type members. So ^^^^^^^^^^^ "represented". Turn the period into a comma. function_type_signature can also be used as a traits bundle. However, this class template should be seen as a white box, I don't know what that means. Please use a more generally understood term, or just drop that part of the sentence. so using the primary interface for clasification and decomposition Drop "using"; you repeat "use" below. is recommended for general use. 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!)? ---- BTW, the use of "Ts" as a parameter is confusing. I keep reading the "s" as a subscript. It's not a list of Ts that's being passed; it's a list of "Types." Why not just use "Types?" --- 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. 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 function_type_signature for *which* type is used? Used how? 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? Members type - The function type as specified by the template arguments. "The synthesized function type" Unless explicitly specified otherwise by Tag, type is a non-variadic function of default calling "convention" . 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. Also, special-case "if" clauses, especially those that test whether something matches a concept (like "is_sequence") tend to destroy genericity. ---- 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. 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. 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++. 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!) -- Dave Abrahams Boost Consulting www.boost-consulting.com