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: Jeffrey Lee Hellrung, Jr. (jeffrey.hellrung_at_[hidden])
Date: 2011-07-16 22:58:13

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.

> 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?
>
[...]

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

* 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