On 7/16/2011 10:58 PM, Jeffrey Lee Hellrung, Jr. wrote:
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.
Appreciated !
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.
I will look into improving the reference and the table.
- 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?
[...]
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.
If I changed it from TTI to something longer I will get complaints that the macro names are too long ( some are already long ). As far as the suggested "Introspection" it does seem vaguer to me than "Type Traits Introspection", byn which I meant to suggest introspecting a type.
* 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.
I totally agree.
* 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.
I will definitely do that. BTW I thought that the double underscores were only considered reserved for the compiler when at the beginning or end of an identifier, but it seems like I am wrong ( haven't found the appropriate comment in the C++ standard ).
* 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.
Agreed.
* I'm confused what the purpose of the *_GEN_BASE and *_GEN macros are, and suspect they are probably unnecessary.
Once the enclosing namespace is removed, I will remove the *_GEN_BASE set and the *_GEN set will become what the current *_GEN_BASE set is. The purpose of the macro is to just generate the name of the metafunction without the end-user having to understand the naming scheme.
* 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?
They do supply a nested 'type', whose 'type::value' is the same as the 'value'. I have not documented this because the nested 'type' is unimportant in using the metafunctions. The metafunction generated by BOOST_TTI_MEMBER_TYPE(name) does have just a nested 'type', which is important and used.
* I think it would be more helpful if the examples were paired with the reference section of the corresponding macro.
Do you mean a link to the appropriate reference section for the macro in the code for the examples ? Or do you mean you want an example added to reference for each 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).
When a nested 'type' is a typedef, the metaprogrammer may want to check if the typedef is some given 'type' That is why the "has type with check" exists.
* 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 told Lorenzo that I will be rolling these into a BOOST_TTI_HAS_TEMPLATE for all 3 cases, which will be slightly different for variadic and non-variadic macro support.
// 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
- 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.
Figuring out the different template parameters and their number at compile-time may be possible. I will look into it.
* 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.
I will have to disagree with your belief that variadic macro syntax should not be used.
* 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.
For functions and data the precision should match that of the compiler without considering default value(s). 'void ( int = 0 )' still has a basic signature of 'void (int) and not 'void ()' so the result should be false. But you have alerted me to provide tests to check out function signatures with default values.
[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.
I will do that.
* Please document in which scopes you may invoke the metafunction-generating macros.
I will do that but let me just reiterate that the metafunction-generating macros generate metafunctions, and therefore anywhere a metafunction can be used is where the macros can be used.
* 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.
Please 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]?
I created them for convenience, because I found them easy to use as an alternative. I do understand now that quote can be used instead, as well of course as placeholder expressions. If there is a strong feeling from others that I should remove the MTFC macros I will do so.
* 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.
Please do elaborate. When I created the nullary metafunctions, as a syntax convenience, I was looking for another way but did not find one.
* It's *awesome* that there's an index...generated by John Maddock's AutoIndex tool?
Yes.
I'm open to discussing the above issues beyond the review period, so, Eddie, you may respond at your convenience.
Feel free to comment further. Eddie