On Thu, Jul 7, 2011 at 5:15 AM, Joel Falcou <joel.falcou@gmail.com> wrote:
The Type Traits Introspection library by Edward Diener started friday 1st have been *extended* to july 17. Comments and reviews are welcome.
[...]
Please explicitly state in your review whether the library should be accepted.
Yes.
The general review checklist:
- What is your evaluation of the design?
Fundamentally sound.
- What is your evaluation of the implementation?
I will try to look into it before the end of the review; specifically, I'd like to compare it to what I have in my own code base.
- What is your evaluation of the documentation?
Generally very good. I think it will be improved if the reference sections for each component also include examples. Also, I think the reference section for each macro is not very clear. I'd prefer something like -------- BOOST_TTI_HAS_TYPE( name ) Description Generates a Boost.MPL-compatible metafunction boost::tti::has_type_'name', with the following semantics. -------- with a table giving all relevant expressions (e.g., boost::tti::has_type_'name'<T>::value), what the parameters are (T is any type), and what the result is (true iff T has a nested 'name' type). I think it's similarly unclear from a quick glance what the types T, U, R, etc. are in Table 1.2 in the "Macro Metafunctions" section. - What is your evaluation of the potential usefulness of the library?
Indeed, very useful; I have similar such metafunction-generating macros that I have used for a while now, and I look forward to replacing them with Eddie's work.
- Did you try to use the library? With what compiler? Did you have any problems?
Not yet.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I've been nonlinearly patching together this review for a few days now, so it might come off as a bit disjointed. I apologize in advance.
- Are you knowledgeable about the problem domain?
I think so.
And finally, every review should answer this question:
- Do you think the library should be accepted as a Boost library?
// ugliest, but most flexible with Boost.MPL - Likewise, I think BOOST_TTI_HAS_STATIC_MEMBER_FUNCTION can subsume BOOST_TTI_HAS_COMP_STATIC_MEMBER_FUNCTION, and likewise I think the syntax has_static_member_function_xxx< T, Return ( Arg0, Arg1 ) > would be convenient. - Although this doesn't have to do with condensing macros, it would make
[...] Again, yes. Ideally, Eddie would accept 100% of my suggestions below, but I am a realistic person (...sometimes): (a) I could be wrong (very wrong!); and/or (b) some issues have multiple legitimate viewpoints. So all I ask is he consider my comments below. First, a general comment. There's a lot more in this library than I expected at first, some of which is welcome but some of which I think might be unnecessary, in that, e.g., it only provides a slight convenience, or only addresses a (in my opinion) obscure use case. You (Eddie) may argue that there's no harm in including it, as users can simply ignore it, but I disagree. I believe there is value in being judicious about what to include and not include. It simplifies the presentation of the library, making it easier to digest; and it reduces maintenance. More detailed comments follow, in roughly the order I thought of them as I read the documentation. Most of them are critical ("Dude, WTF, you should do X instead of what you're doing now"), and I apologize in advance for not mentioning the things I believe you did correctly. I just want to get straight to the point of likely the most contentious and discussable issues :) * I would suggest a name change from "TTI" to "Introspection" or something similar. I have a feeling that, looking at a source file with "#include <boost/tti/header.hpp>" would give one less of an idea of what the purpose of that header is than "#include <boost/introspection/header.hpp>", and likewise for macros, "BOOST_TTI_MACRO" vs. "BOOST_INTROSPECTION_MACRO". I don't think the length of the name should be a huge concern since I would imagine the ratio of metafunction-generating-macro use to generated-metafunction use is much less than 1, i.e., the macro will not be used frequently relative to the metafunction that's generated. * As others have noted and as I believe Edward has agreed to, generated metafunctions should be injected into the scope of the macro invocation, not in the boost::tti namespace. * Contrary to others opinions, I think Edward should keep both BOOST_TTI_MACRO and BOOST_TTI_TRAIT_MACRO macros (optionally renaming them, if desired; I think these are just following the Boost.MPL naming scheme, but perhaps we can think of names that evoke the difference a little more clearly). It imparts a de facto naming convention for these metafunctions. *But* you should include a warning about the possibility to automatically generate names with double underscores, and that this behavior is undefined. * As others have noted, the association between file names and the macros they define is confusing, so I think we should strive for a more predictable association. * I'm confused what the purpose of the *_GEN_BASE and *_GEN macros are, and suspect they are probably unnecessary. * I *think* all generated metafunctions should also expose a nested "type" typedef in addition to a nested "value" static bool in order to be fully Boost.MPL compatible, but...I'm not sure on this one. For some reason, I've always followed this practice, I think because it allows has_type_xxx<T> to be used as a nullary Boost.MPL metafunction. Can someone comment on this? * I think it would be more helpful if the examples were paired with the reference section of the corresponding macro. * I don't see a compelling use case for "has type with check", where "check" checks if the type is the same as some given type. I would think it would be more useful for "check" to evaluate some Boost.MPL metafunction (and the old behavior is arguably clearer now: boost::is_same< U, boost::mpl::_1 >), and then you can roll this functionality into the "no check" TTI metafunction by defaulting the MPL metafunction template parameter to boost::mpl::always< boost::true_type > (or similar). Indeed, I *think* this would remove the need for BOOST_TTI_MEMBER_TYPE for the use case you present (see below). * I like Lorenzo's idea to condense some related macros into a single macro: - I think BOOST_TTI_HAS_TEMPLATE can subsume BOOST_TTI_HAS_TEMPLATE_CHECK_PARAMS and BOOST_TTI_VM_HAS_TEMPLATE_CHECK_PARAMS. - I think BOOST_TTI_HAS_MEMBER_FUNCTION can subsume BOOST_TTI_HAS_COMP_MEMBER_FUNCTION, since I think you could just dispatch on whether the first template parameter to the generated metafunction is a pointer-to-member-function or not. Another convenient syntax may be has_member_function_xxx< T, Return ( Arg0, Arg1 ) >, so that all of the following are equivalent: has_member_function_xxx< Return (T::*)( Arg0, Arg1 ) > has_member_function_xxx< T, Return ( Arg0, Arg 1 ) > // can still use lambda expressions for T, has similar format as other introspection metafunctions has_member_function_xxx< T, Return, boost::mpl::vector2< Arg0, Arg1 the library a little more consistent to allow pointer-to-member syntax for BOOST_TTI_HAS_MEMBER_DATA, e.g., has_member_data_xxx< U T::* >. - Lastly, BOOST_TTI_HAS_STATIC_MEMBER_FUNCTION( xxx ) can be reexpressed as BOOST_TTI_HAS_MEMBER_FUNCTION( static xxx ), and likewise for BOOST_TTI_HAS_STATIC_MEMBER_DATA. * For BOOST_TTI_HAS_TEMPLATE where you supply the template signature, I'm actually fine with the old system of wrapping the signature in parentheses and replacing all commas with ")(" to make a Boost.PP Seq, and prefer it to the array syntax you've proposed since. That said, I would probably, ultimately, prefer Lorenzo's syntax (e.g., "TTI_HAS_TEMPLATE( template( class, class ) struct tmpl )"), but I guess we'll agree to disagree on what's easier to read (readability should win over writability, right?). For the syntax taking advantage of variadic macros that you've proposed during the review (e.g., "TTI_HAS_TEMPLATE( tmpl, template< class, class > )"), I worry about usability issues when you're just stringing the template signature into the __VA_ARGS__ argument. I don't use variadic macros yet, so my sense of "good practice" is very malleable at the moment, but, that being said, it seems like good practice is to use variadic macro arguments only when (a) your macro logically takes a fixed number of arguments, but where you can syntactically leave off trailing arguments that you wish to be defaulted; or (b) your macro takes a varying number of arguments, but the semantic difference between each argument is purely and strictly positional. To me, the template signature is a completely separate argument from the template name, and the syntax should reflect that, e.g., TTI_HAS_TEMPLATE( tmpl, (template< class, class >) ) or TTI_HAS_TEMPLATE( tmpl, ( class, class ) ). It also seems like this would make it easier to use TTI_HAS_TEMPLATE within other preprocessor metaprogramming constructs (e.g., if I wanted to generate an entire family of metafunctions via BOOST_PP_REPEAT or BOOST_PP_ITERATE), but perhaps you can argue otherwise. * When introspecting for nested templates and member functions, how "precise" is the result? I.e., if you're checking if T has member function xxx with signature void ( ), and T in fact has a member function xxx with signature void ( int = 0 ), is the result false? (I'm guessing yes, and this should be noted in the reference.) Similarly for nested templates with default template parameters. [The next three comments have been echoed by others.] * Please document is how the accessibility of the inner elements affects whether metafunction invocations can be compile and what their result is. * Please document in which scopes you may invoke the metafunction-generating macros. * Please provide rationale for why you support introspection some properties but not others (e.g., namespace introspection, virtual functions). I assume this is a limitation of the C++ language. * The use case you give to motivate BOOST_TTI_MEMBER_TYPE and valid_member_type seems somewhat contrived. I have never run into this situation further than nesting 1 level deep. However, assuming the validity of the use case, if you have a has_type_xxx< T, Predicate > metafunction, you can determine whether T::AType::BType::CType::FindType exists without a compiler the error in the event that one of the intermediate types doesn't exist via has_type_AType< T, has_type_BType< boost::mpl::_1, has_type_CType< boost::mpl::_1, has_type_FindType< boost::mpl::_1 > > >
Oops, okay, that won't work without some additional boost::mpl::protect's, but hopefully you get the idea. Let me know if you want me to elaborate. * Similar to above, I'm not convinced of the utility of the metafunction-class-generated macros...aren't they largely obviated by boost::mpl::quote[n]? * Lastly, and I'm not absolutely sure about this, but it seems like adding metafunction predicate parameters to the has_type_xxx generated metafunctions would obviate the need for the nullary metafunction infrastructure. I can elaborate on this further if you don't understand what I mean. * It's *awesome* that there's an index...generated by John Maddock's AutoIndex tool? I'm open to discussing the above issues beyond the review period, so, Eddie, you may respond at your convenience. - Jeff