Hi Jody, Jody Hagins wrote:
First, this looks very cool, and I think it will get a lot of use even from those who are a bit scared of full MPL.
This is not my official review, but simply some questions/comments about the documentation, specifically the things I think of as I read it for the first time...
I am not a specialist in grammar, and I have the distinct disadvantage of American English as my native language (well, most will say that South Carolina-ese is English). However, I had to read the first sentence of the MOTIVATION section three times to decipher its meaning. I think it could be better understood if it were restructured...
OK. All I have to say for my excuse is that English is only my secondary language... So, if the first sentence is hard to understand it should be changed.
Also, in the first paragraph of MOTIVATION, the author asserts "common applications" and "well known special cases" of which a large number of people are not familiar. IMO, if you reference a motivating case, you should explain it a bit more, or provide references to examples.
Good point.
I am similarly confused by the last paragraph of MOTIVATION. What is meant by "It covers the functionality of the function_traits class?"
It means that (given the your compiler is supported by the library) all that can be done with Boost.TypeTraits' function_traits class (and more) can also be done with the FunctionTypes library.
After reading the OVERVIEW and MOTIVATION, I am still at a loss for what benefit this library will provide. I can make some reasonable assumptions, but I would think that I should have a much better picture than I currently have from reading those two sections.
What exactly is unclear ?
Obviously, I am either a total idiot, or I am just having a bad day (please do not confirm the former if you know me personally). I am having a hard time groking the docs for CLASSIFICATION, especially the summary. I do not think I should have to read the full description to get an idea of what the function does, yet after reading the summary, I thought, "Hmmm... maybe a read through the details will help." To me, this indicates a little more information should be in the summary.
Also, the CLASSIFCATION docs say that it tests to see if a type is an element of a set of other types, and gives several .cpp files as examples. However, after reading the entire specification for is_function_type, I have these questions...
How do I build a set of types? How do I test for a type in a set? All examples show one type, not a set of types.
What should happen if I put types in the set that are not function types? Should I expect a compile error, or is it up to the programmer to know specifically that the types belong in that set?
Here's a fundamental misunderstanding: There are no sets of types that can be _built_. You just _name_ a set of possible kinds of function types to test against by specifying a tag: is_function_type< function_pointer, T > Tells me if T is contained in the set of "all function pointers", which is described by the tag type function_pointer. I can ask for a superset (all types handled by the library) like this: is_function_type< any_function, T > Or a subset: is_function_type< variadic_function_pointer, T > Did you read the Tags section at all ? Was it unclear ? Where ?
For the function calls, the descriptions make some assumptions (like, it must be called with a function type). Are these suggestions for good style, or will the implementation provide a compile-time assertion, or will you ger undefined behavior if you violate the assumption?
If you violate the preconditions the behaviour is generally undefined. However, the type members won't be defined to allow better diagnostics for client code (this is currently undocumented).
function_type_arity: Should probably put note about BOOST_FT_MAX_ARITY here (or reference the CONFIGURATION section) to make it obvious early about the max arity.
BOOST_FT_MAX_ARITY applies to the whole library. Still, a reasonable point.
What happens if you call it with a non-function? Do we get a compile time assertion?
Another violated precondition. We had this above, already.
function_type_parameter: should probably say that the index starts at 0 (this is the common C/C++ method, but stating it explicitly would be helpful).
Agreed.
It would also be helpful to be a bit more explicit about the difference between function_type_parameter and function_type_parameter_c, since not everyone groks MPL as second nature.
Well, in case you don't know MPL (although the primary audience most likely will) you can still have a look at the synopsis and look for the version with non-type template parameter...
How do functors fit in here? If a class overloads operator(), is it considered a function type?
No. Is the definition really that unclear ? How could this probably work if operator() is a function template ? You can decompose a member function pointer to a parentheses operator (which implies it's already instantiated if it's a template function), though (after deduction or using some kind of typeof operator).
In the ENCAPSULATION section, I think the author is again making too many assumptions about the detailed knowledge of MPL. This library, while using the MPL, and imitating some aspects, does not require full
This _part_ of the library provides an MPL Sequence interface. There is really no point in using it without knowing the MPL and its algorithms. Yes, you can use the library without knowing MPL, but this particular part is about MPL interoperability. Sorry !
knowledge of the MPL. However, the docs seem to take too much for granted, and make too many assumptions. I'd like to see more detail, especially in the descriptions.
What do you mean with "assumption" ? Preconditions on template arguments ? As this library's classes are _slightly_ more complex in it's use than the ones of Boost.TypeTraits (in some light it can be seen as an extension) what makes you think there has to be so much more detail ?
After my initial reading, I am still not entirely sure about function_type_signature. I would like to see more description information. Also, I do not like the "types" member. What is the rationale for the ordering of the types (return, [class], parameters)? Callers would have to query even more information to determine how to interpret types.
These members are documented at all because they might be helpful in some rather rare cases - I should probably remove some of them from the documentation. The primary interface of function_type_signature<T>, however, is to use it as an MPL-Sequence.
Also, if you remove c-v qualifiers, how can a caller ever get what was truly passed to function_type_signature? Maybe adding something like raw_representee, or something else so that the caller can see the true T?
Very good point - this is unclear: it refers to top-level cv qualification of pointers. E.g: void(*const)(); The cv-qualification of "member function pointees" is _never_ dropped.
function_type: looks very intetresting. Unfortunately, none of the thoughts/questions in my head about usability are answered by the examples.
PORTABILITY: If you have not tested with BCB, maybe you should (or get someone to do it). Or, at least note that it is most likely broken on that compiler, w.r.t. function references. Specifically, BCB6 does not properly distinghush...
BCC is broken for all sorts of reasons. It is _not_ supported and does _not_ speak enough ISO-C++. Btw. I know a workaround for the problem regarding function references but the lack of partial specialization for member function pointers is much worse. I guess Boost.Typeof could be used to work around it (once it supports Borland), but I'm really not sure it's worth putting much energy into supporting this compiler (yes, I know BCB is cool for quickly building user interfaces) as the product suffers halfhearted maintainance and probably won't be around for very much longer.
int (func)() int (*ptr_to_func)() int (&ref_to_func)()
Should the CONFIGURATION section go before the detailed descriptions of the function calls?
Should it ?
In general, I do not like using test assertions as examples. I know lots of boost stuff uses this methodology, but it does not provide much information. If tests are used, then they should include LOTS of documentation describing exactly what is (and is not) happening, and sufficient justifications.
I never had problems with "assertions as examples" as a user...
After reading the docs, I think many will be at a loss for concrete examples of real-world use for this library.
It factors out existing Boost functionality in components like Bind, Lambda, ResultOf, Phoenix.... I do not want to change around these components (the library authors are free to do so of course - and I guess, and hope, some will welcome it), however, there are many more specialized situations it would be necessary to duplicate the redundancy in these components yet another time. How about a callback registration that knows from the signature of the registered function the data to call it with ? (This was my reason to need the functionality of this library) Component writers who use this library also get the benefit of a central point of configuration and being better neighbours with other components which need the same functionality in terms of compilation speed.
Again, in a library like this, the documentation should include LOTS more examples, and each example should be fully documented. Otherwise, the curve to usage is enormous.
How could someone unfamiliar with template metaprogramming (with or without using MPL) possibly be interested in using this library ? So I'm not sure you are among the intended audience. And I really would love to hear more voices from those who undoubtfully are, on what _exactly_ needs to be improved.
Finally, I'd like to see some examples that support the motivation. The author cites some reasons for the library to exist. I want proof, in the form of examples that show how difficult (or impossible) it is to do what he wants without the library, and then examples showing how the
E.g. let's extract the first parameter of a member function pointer type: Currently there is no way to do it except use Boost.Preprocessor to create a cascade of partial template specializations for every possible arity both with and without ellipsis for every calling convention required (or write it by hand, if you like ;-) ).
same problem is easily solved with the library. I do not think this is just unique to this library. I think it is important for all libs, hint... hint...
With the FunctionTypes library the above scenario looks like this: function_type_parameter_c<T,2>::type and it will compile as fast as the hand written version (the library provides preprocessed headers - and is probably even faster because plain function types and function references are transformed to pointers before using specialization to match the type). Thank you for the huge ammount of feedback ! Regards, Tobias