Re: [boost] [TTI] Review for The Type Traits Introspection library by Edward Diener **extended**

Subject: Re: [boost] [TTI] Review for The Type Traits Introspection library by Edward Diener **extended**
From: Edward Diener (eldiener_at_[hidden])
Date: 2011-07-17 11:10:26

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_at_[hidden]>  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.

>      - 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
>>> // 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
> 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