Re: [boost] [TTI] Review

Subject: Re: [boost] [TTI] Review
From: Edward Diener (eldiener_at_[hidden])
Date: 2011-07-10 23:59:03

On 7/10/2011 7:59 PM, Lorenzo Caminiti wrote:
> Review of Boost.TTI 2011-07-10 (Lorenzo Caminiti)
> =================================================
>
>
> Do you think the library should be accepted as a Boost library?
> ---------------------------------------------------------------
>
> Yes, in my opinion Boost.TTI should be accepted as a Boost library.

Appreciated !

> Key points are:
>
> 1) I would essentially found this library useful as is.
>
> 2) The library should not expand its macros into the boost::tti
> namespace (I give a few reasons and suggest alternatives below).
>
> 3) The library uses too many macros to provide essentially the same
> functionality (with minor variations). This is confusing and the
> library macro API should be simplified (I suggest a possible
> simplification below).

I will answer each of your points below rather than generally answering 
2) or 3) above.

>
>
> My comments are all numbered and marked as follow:
> [MUST] If these comments are not addressed, I would no longer
> recommend to add this library into Boost.
> [WANT] I would like to see these comments addressed but I would still
> recommend to add this library into Boost even if these comments are
> not addressed.
> [NOTE] I do not feel strongly about these comments and the authors can
> ignore these comments if they wish to do so.
>
>
> What is your evaluation of the design?
> --------------------------------------
>
> 1.  [MUST] The library provides too many macros (with very similar
> functionality) which make the interface difficult to grasp.
> The authors should take some time to try to redesign and simply the
> library interface ideally reducing the number of macros.
>

I will give my reasons for each of the macros in answer to your 
suggestions below, but I completely agree that if the number of macros 
could be simplified and still offer the same basic functionality it 
should be done.

> [WANT] I would reduce the macro API to the following 5 macros (no
> more, as I explain in the rest of the comments below):
>
>     HAS_TYPE(has_mytype, mytype)
>     HAS_TEMPLATE(has_mytpl, [template(...) {class|struct}] mytpl)

I can look into combining the various template macros if it can be done. 
Currently there are three variations, disregarding the complex form of 
each macro. The variations are:

BOOST_TTI_HAS_TEMPLATE(name)
BOOST_TTI_HAS_TEMPLATE_CHECK_PARAMS(name,pp-seq-of params)
BOOST_TTI_VM_HAS_TEMPLATE_CHECK_PARAMS(name,variadic-sequence-of-params)

You would like to see a single macro, called BOOST_TTI_HAS_TEMPLATE, 
which could alternately take a pp-seq-of params or a 
variadic-sequence-of-params. I agree that would be wonderful if it could 
be done.

To do this the compiler must support variadic macros AFAICS. What if the 
end-user's compiler does not do so ? Even if the end-user's compiler 
supports variadic macros, how do I tell the difference between a pp-seq 
of params and a variadic-sequence of params ?

So here are my thoughts. If I supported the single macro for compilers 
which support variadic macros, I still need to support the two 
non-variadic macro versions for compilers which do not support variadic 
macros.

For the variadic macro version I can find out that is the 
variadic-sequence-of-params if it has more than one variadic parameter 
after the name. If it only has one variadic parameter after the name I 
can check if the parameter starts with a set of parens and, if it does, 
it is the pp-seq of params, else it is the variadic-sequence of params. 
I think this is doable. But I still can not see my way around dropping 
support completely for the non-variadic versions of this macro.

>     HAS_MEMBER_VARIABLE(has_myvar, [static] myvar)
>     HAS_MEMBER_FUNCTION(has_myfunc, [static] myfunc)

I can easily combine BOOST_TTI_HAS_MEMBER_DATA and 
BOOST_TTI_HAS_STATIC_MEMBER_FUNCTION into HAS_MEMBER_VARIABLE, which 
covers either case, and BOOST_TTI_HAS_MEMBER_FUNCTION and 
BOOST_TTI_HAS_STATIC_MEMBER_FUNCTION into a HAS_MEMBER_FUNCTION, which 
cover either case. But then the end-user will lose the ability to 
distinguish between a 'member data/static member data' or 'member 
function/static member function'. Do you really think that is the 
correct thing to do just because you think there are too many macros ? I 
do not.

OTOH I do not mind adding the combined member data and member function 
macros as you suggested, while keeping what already exists, but then 
that adds more macros ( which does not bother me a bit ), which you do 
not like.

See below for my comments about the composite type functions.

>     MEMBER_TYPE(trait, name)
>
> 2.  [MUST] Expanding the metafunctions within the boost::tti namespace
> presents a number of serious issues:
> a) If multiple part of the program (possibly independent libraries)
> use the TTI library to introspect the same name, the metafunction
> names will clash.
> b) The library macros can only be used at namespace scope. However, it
> should be possible to use the macros also at class scope so to define
> the introspection metafunctions as inner classes, etc (this is
> critical for example in my application for Boost.TTI where I use it to
> implement Boost.Contract and all Boost.Contract macros expand at class
> scope).

> The authors should address these issues.

I totally agree with your criticism here and I will remove the generated 
macros from any namespace. Others have also mentioned the same thing and 
I immediately realized they were right when I read their reasoning.

>
> [WANT] To address these issues, I would suggest to simply define the
> metafunctions within the enclosing scope. It will be the user's
> responsibility to use the macros within the correct scope (namespace,
> class, etc). For example:
>
>     template<  class T>
>     struct OurTemplateClass {
>         BOOST_TTI_HAS_TYPE(has_mytype, _mytype) // also at class scope
>
>         void f() {
>             std::cout<<  has_mytype<T>::value<<  std::endl;
>         }
>     };
>
> This should also allow to remove all the GEN/GEN_BASE macros.

I can remove the GEN_BASE set but I would still keep the GEN set ( which 
would be the same as the current GEN_BASE set ) in order to provide an 
easy way to generate the metafunction name from the appropriate macro, 
without the end-user having to manually remember the algorithm by which 
I generate the metafunction name.

See below for my comments about generating the metafunction name.

>
> 3.  [MUST] The library macros prefix the specified name with _ but
> this can create a symbol with double underscores __ which is reserved
> by C++ (e.g., HAS_TYPE(_my_type) creates a metafunction named
> boost::tti::has_type__my_type which is a reserved symbol).
> The authors should address this issue.

You have made a good point and I can simply remove the preceding '_' 
when generating the final metafunction name.

>
> [WANT] To address this issue (and also to reduce the number of macros
> eliminating the need for separate TRAIT macros), I would suggest to
> always ask the user to specify both the trait and the introspected
> name (not just the introspected name) and then use the trait to name
> the metafunction (as always, it is the user's responsibility to not
> use reserved names). For example:
>
>     HAS_TYPE(has_mytype, _mytype) // generates metafunc has_my_type
>
> This should also allow to remove all the TRAIT macros.

You want to remove functionality for automatically generating a macro 
metafunction name just because you feel there are too many macros. I 
believe that the automatic generation of the metafunction name is 
welcomed by metaprogrammers. The MPL macros 
BOOST_MPL_HAS_XXX_TEMPLATE_DEF/BOOST_MPL_HAS_XXX_TEMPLATE_NAMED_DEF and 
BOOST_MPL_HAS_XXX_TRAIT_DEF/BOOST_MPL_HAS_XXX_TRAIT_NAMED_DEF follow the 
scheme I have chosen.

>
> 4.  [WANT] I really think that mixing () and<>  parenthesis is confusing:
>
>     HAS_TEMPLATE_CHECK_PARAMS(MoreParameters,
>         (class) (class) (int) (short) (class)
>         (template<class)(int>  class InnerTemplate) // confusing :(
>         (class)
>     )
>
> IMO, this would be more readable as the following:
>
>     HAS_TEMPLATE_CHECK_PARAMS(MoreParameters,
>         (class) (class) (int) (short) (class)
>         (template( (class) (int) ) class InnerTemplate) // ok :)
>         (class)
>     )

I will look into this. It is obviously more difficult programming with 
the latter than the former, but it is probably doable although with much 
effort. It may not be worth the effort.

What the former syntax reflects is an exact transcription of the 
template parameters with each comma replaced by ')(' and a starting and 
ending parentheses. So for the template:

'template <class,class,int,class,template <class> class 
InnerTemplate,class,long> struct ManyParameters { }'

the parameters are:

'(class)(class)(int)(class)(template <class> class 
InnerTemplate)(class)(long)'

and all I had to do as an end-user was copy the template parameters, add 
a '(' at the beginning, add a ')' at the end, and change every ',' to a 
')('. As a programmer I take the sequence and directly convert it to the 
template parameters via a BOOST_PP_SEQ_ENUM.

>
> 5.  [NOTE] There is no real need to use another macro ..._CHECK_PARAMS
> because HAS_TEMPLATE can be reused. This would reduce the number of
> macros. For example, with template parameters:
>
>     HAS_TEMPLATE(
>         template( // gives the parameters (optional)
>             (class) (class) (int) (short) (class)
>             (template( (class) (int) ) class InnerTemplate)
>             (class)
>         )
>         struct MoreParameters // struct or class can be used here
>     )
>
> And the same macro without template parameters:
>
>     HAS_TEMPLATE(MoreParameters)

See above for my answer to this.

>
> 6.  [NOTE] The same macros can accept both pp-sequences and variadic
> pp-tuples (when variadic macros are supported). This eliminates the VM
> macros reducing the number of different macros in the library API. For
> example:
>
>     HAS_TEMPLATE(
>         template(
>             class, class, int, short, class,
>             template( class, int ) class InnerTemplate, // variadic commas
>             class
>         )
>         struct MoreParameters
>     )
>
> I know this uses template( class, int ) instead of template<  class,
> int>  but this way the syntaxes with and without variadics are unified
> and consistent (and more readable IMO). Alternatively, you can
> probably program the macro with variadic to be:
>
>     HAS_TEMPLATE(
>         template<  // template<  >  here
>             class, class, int, short, class,
>             template<  class, int>  class InnerTemplate, // template<  >  here
>             class
>         >
>         struct MoreParameters
>     )
>
> But still keep the following syntax without variadics:
>
>     HAS_TEMPLATE(
>         template( // template( ) here
>             (class) (class) (int) (short) (class)
>             (template( (class) (int) ) class InnerTemplate) // template( ) here
>             (class)
>         )
>         struct MoreParameters
>     )

See above for my answer to this.

>
> 7.  [WANT] Can the authors explain why the inner template parameter
> name InnerTemplate is needed while all other template parameter names
> are not needed? Is it really needed? Why I cannot do:
>
>     HAS_TEMPLATE(
>         template(
>             class, class, int, short, class,
>             template( class, int ) class, // no name InnerTemplate
>             class
>         )
>         struct MoreParameters
>     )

It should not be needed. Please try without it. If it does not work I 
will look and see why and attempt to correct it.

>
> 8.  [NOTE] There is no need to use a different set of macros for
> static because the keyword static can be used to have the pp detect
> this case. This will reduce the number of different macros. For
> example:
>
>     HAS_STATIC_MEMBER_FUNCTION(has_static_f, f)
>
> Can be replaced by:
>
>     HAS_MEMBER_FUNCTION(has_static_f, static f)

See above for my previous discussion on dropping the distinction between 
member functions and static member functions.

>
> 9.  [WANT] Why can't we always use the composite function syntax R
> (C::*)(A0, A1, ...)? This is similar to the preferred notation for
> Boost.Function so if possible it should be used instead of the other
> notation.
>
> As I understand it, some compilers have troubles supporting the
> composite syntax. However, these compilers might not be able to
> support the TTI library all together. Unless there is evidence of
> compilers that can support TTI but not the composite function syntax,
> I think only the composite function notation should be provided.
>
> This should also allow to remove all the COMP macros.

The reason for having both a composite syntax, which you like, and the 
non-composite syntax of individual types is:

1) The composite type syntax can be used when one has to pass a calling 
convention, or possible some compiler-specified function-like syntax, 
which the function_types 'tag' type does not support. I think this is 
absolutely necessary to have.

2) The individual type syntax is there so that MEMBER_TYPE can be used 
to pass a nested type which does not have to exist without causing a 
compiler error. I also thin this is absolutely necessary to have.

Please read the section called 'Nested Types' in the documentation to 
understand why MEMBER_TYPE is used. The ability to pass a nested type in 
the form of a MEMBER_TYPE as a function parameter is very important 
piece of functionality. Being able to do this for a function parameter 
type or for the function return type is something I do not want to 
eliminate.

>
> 10. [NOTE] I think "member variable" is a more accepted name that
> "member data"-- isn't it? If so, I'd rename MEMBER_DATA to
> MEMBER_VARIABLE.

I can understand your preference. I will consider it.

>
> 11. [WANT] Is it possible to have some sort of composite syntax for
> member variables?

There is no reason to have a composite syntax for member variables. A 
member variable is a single type.

> I remember reading something about this for
> overloading the operator ->* to access member variables (as well as
> calling member functions) but I can't double check right now... can
> the authors look into this? For example:
>
>     HAS_MEMBER_VARIABLE(has_number, number)
>
>     has_number<T::short>  // composite form for member variable?
>
> If there exists a composite form for member variables, I would like
> the generated metafunctions to use it instead of the plain form (so to
> be consistent with the MEMBER_FUNCTION macros-- see comment above).
>
> 12. [WANT] Are the metafunction MTFC macros really needed? The docs
> say they reduce compile-time (compared to using MPL placeholders) but
> no evidence is shown to support that...
>
> Extra macros complicate the library API so if they are provided for
> reducing compile-time there should be some benchmarking showing their
> benefits. If not, these macros should be removed.
>
> [NOTE] If these macros are shown to be beneficial, I'd rename them
> with a METAFUNC postfix because I find it more readable. For example:
>
>     HAS_TYPE_METAFUNC
>     HAS_MEMBER_FUNCTION_METAFUNC

I supplied them merely as a convenience. In an early mailing list 
message about the TTI library from Dave Abrahams, before this review, he 
suggested that passing metafunctions as data as a metafunction class 
would generally be faster than passing them via placeholder expressions. 
I do not mind eliminating them if it seen as overkill.

>
> 13. [WANT] Are the nullary metafunctions really needed?

Functionally, no, which the documentation explicitly explains. 
Syntactically I feel they are much easier to use once they are understood.

> The docs say
> they simplify the syntax but no side-by-side example comparing the
> syntax with and without the nullary type metafunctiosn is provided...
>
> Unless their benefit is clearly shown, the nullary metafunctions
> should be remove to simplify the library API.

I will add side by side examples showing the simpler syntax of using the 
nullary metafunctions. I do show the different syntaxes for similar 
situations in the doc, but not side by side.

Why not just ignore them if you do not like their syntax ? After all 
tghey are just a set of metafunctions, which no one has to learn to use 
if they do not want to do so. They do not interfere with anything else 
in the library and they allow specifying nested types in a syntactically 
easier way.

>
> 14. [NOTE] I'd fully spell the header file names to improve
> readability and be consistent with the macro names (which are
> correctly fully spelled). For example, "member_function.hpp" instead
> of "mem_fun.hpp".

I can do that, and probably will. You have a good point.

>
> 15. [NOTE] I'd rather use an actual name for the library instead of
> the cryptic acronym TTI (but I personally don't like the widely used
> MPL neither so maybe it's just me). Maybe "intro" (for introspection),
> or "mirror" (look at yourself), or "soul" (look into your soul), or
> "psycho" (look into your person) ;) would be better names...

Where would you like to see the longer name ? I certainly do not mind 
calling the library Type Traits Introspection but that is quite a 
mouthful. Like MPL ( for Metaprogramming Library ), TTI is easier to 
remember and refer to.

>
>
> What is your evaluation of the implementation?
> ----------------------------------------------
>
> 16. I did not look at the implementation.
>
>
> What is your evaluation of the documentation?
> ---------------------------------------------
>
> 17. [WANT] I'd add a "Motivating Example" in the Introduction section
> to very briefly illustrate the library functionality (probably right
> after the bullet list on the library functionalities). I had to go all
> the way into the Nested Types section to start seeing some code... (I
> was motivated just because I knew what the library does and I've used
> MPL_XXX macros with SFINAE before).

I totally agree with you. I intend to revamp the introductory material 
to make it simpler and easier to understand what the library can do, and 
present some basic motivating examples.

>
> 18. [NOTE] I don't think you need a space before "?" in English (see
> "Why the TTI Library ?", etc).

You are right. I will change it. It is just my style but unnecessary.

>
> 19. [WANT] Typo in "depending on how many parameters are bring passed".

Corrected ! Thanks !

>
>
> What is your evaluation of the potential usefulness of the library?
> -------------------------------------------------------------------
>
> 20. [NOTE] The library is very useful as is. For example, I use a
> similar introspection technique in Boost.Contract as part of a
> mechanism to check if a base class has a virtual function that is
> being overridden so to automatically subcontract from such a base
> class function.
>
> 21. [WANT] I think the docs should add an annex with a complete list
> of all traits that would ideally be introspected even if they cannot
> actually be introspected. For example, a table could list all traits
> that would be good to introspect and then for each trait say
> "introspected by this library", or "cannot be introspected in C++
> because...", etc. For example, Boost.Contract could use the ability to
> check if a function is public, protected, or private to simplify its
> macro syntax (because only public functions shall check class
> invariants).
>
> Possible traits for introspection I can think of are: is public, is
> protected, is private, is template, has these template parameters, is
> explicit, is inline, is extern, is static, is virtual, is const
> member, is volatile member, has these exception specifications
> (throw), any more?

I think your possible list is too arbitrary. Most anything can be added 
here.

>
> 22. [WANT] I'd add an annex to the docs to compare this library with
> other libraries (e.g., the "mirror" library?) that exist out there for
> introspection (i.e., some sort of literature review).

if there were an existing Boost library I might do it.

> I would also
> comment on possible C++ standard extensions or compiler specific
> support for introspection.

Which C++ standard ( 2003 or C++0x ) ? For the latter I still have much 
to learn since it is so new.

>
>
> Did you try to use the library? With what compiler? Did you have any problem?
> -----------------------------------------------------------------------------
>
> 23. Yes, I compiled all the examples from the docs and played around
> with the library a bit more. I have used GCC 4.3.4 on CygWin and I had
> no issue.
>
>
> How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?
> --------------------------------------------------------------------------------------------
>
> 24. I'd say in between a quick reading and an in-depth study. I spent
> about 12 hours total reading the docs, trying the examples, and
> writing this review.
>
>
> Are you knowledgeable about the problem domain?
> -----------------------------------------------
>
> 25. I have used SFINAE before to introspect if a class has a given
> inner class to implement subcontracting for Boost.Contract.
> Furthermore, I have used template metaprogramming in a number of
> occasions.
>
> Overall, I'd say that I am familiar with the problem domain but I am
> not an expert.
>
>
> --Lorenzo

Thanks very much for your review and specific comments.

Eddie Diener