Re: [boost] [Review] Type Traits Introspection library by Edward Diener starts tomorrow Friday 1

Subject: Re: [boost] [Review] Type Traits Introspection library by Edward Diener starts tomorrow Friday 1
From: Edward Diener (eldiener_at_[hidden])
Date: 2011-07-05 11:35:28

On 7/5/2011 4:38 AM, John Maddock wrote:
> OK here's my review of this library.

Thanks for the review.

>
> First off the headline - yes I believe it's a useful library that should
> be accepted into Boost, subject to some of the comments below.

Appreciated.

>
>> - What is your evaluation of the design?
>
> Probably too many macros - see detailed comments below, otherwise fine.

You know what Mozart said to the Emperor Joseph when the latter said 
there were "just too many notes in 'The Marriage of Figaro'", don't you 
? <g>.

>
>> - What is your evaluation of the implementation?
>
> The library is more or less a meta-library around MPL (or else uses
> MPL-like implementation techniques, and IMO that's the right way to go.
>
>> - What is your evaluation of the documentation?
>
> Nice and complete, but confused me in places (see below).
>
>> - What is your evaluation of the potential usefulness of the
>> library?
>
> Very useful, I could have used this a few times in the past.
>
>> - Did you try to use the library? With what compiler? Did you
>> have any problems?
>
> There are issues with the Intel compiler which Edward is investigating -
> see also detailed comments below
>
>> - How much effort did you put into your evaluation? A glance? A
>> quick reading? In-depth study?
>
> Full read of the docs, and about an hour playing with the code. Will
> spend some more time getting to the bottom of the Intel issue shortly.
>
>> - Are you knowledgeable about the problem domain?
>
> As I maintain the type traits lib I hope so, though I confess to *not*
> being a hard core meta-programmer :-0
>
>> - Do you think the library should be accepted as a Boost library?
>
> Yes, subject to the comments below.
>
> Detailed comments:
> ~~~~~~~~~~~~~
>
> 1) I agree with the other comments that it's basically just plain wrong
> to insert the generated traits classes in a namespace: they should
> appear within the current namespace scope. Under "General Functionality"
> the "Important" block should (IMO) encourage users to place macro
> invocations within a private "detail" namespace to avoid name clashes
> from multiple use in multiple headers.

I agree with the criticism by others that it was wrong to put the 
generated metafunctions in a namespace and I will remove that. I will 
add documentation which explains further about the possibility of name 
clashes for the generated macros and why users of the macros should 
invoke the macros in a namespace which will not clash with other users 
or their own names.

> 2) Further to the above, I wonder if it's worthwhile providing a set of
> "standard" introspection classes for the common cases, a straw man list
> would be something like:
>
> Types: type, value_type, size_type, difference_type, iterator,
> const_iterator, pointer, const_pointer, reference, const_reference.
> Static Values: value
> Functions: begin(), end(), swap(self_type)

That can be easily done once it is determined what to cover.

>
> 3) I was confused by the section "Macro Metafunction Name Generation", I
> think some examples would help a lot. Does the variety of macros
> supplied get simplified by just dumping the generated trait in the
> current namespace?

I will move the explanations in that section near the beginning of the 
subsequent topic on "Macro Metafunctions" and explain it better.

I had adopted the idea of the macro metafunction name generation macros 
fairly late in the development when I realized that it would be easier 
creating such macros directly based off of the name of the macro 
metafunctions themselves rather than asking the end-user to remember a 
naming scheme.

When I remove the putting of the macros metafunctions in the boost::tti 
namespace the set of macro metafunction name generation macros will be 
cut in half.

The basic idea of the name generation macros is that for a macro 
metafunction named, as an example, BOOST_TTI_HAS_TYPE the metafunction 
name generated is given by BOOST_TTI_HAS_TYPE_GEN. In other words the 
metafunction name for a macro metafunction X(name) is X_GEN(name). This 
will be part of the better explanation.

>
> 4) BOOST_TTI_MEMBER_TYPE looks like it would be more useful (to me
> anyway!) if the generated template would accept a second optional
> parameter that is the value of the ::type member when the type being
> checked does not have the specified inner type (hope that makes sense).

Yes, it makes sense. I can certainly add this with no problem, but I am 
wondering what the use case for this situation would be. The use of 
BOOST_TTI_MEMBER_TYPE is simply to pass a type which may not actually 
exist as a nested type, and let the TTI metafunctions operate with such 
a type without generating a compiler error. Also the end-user can always 
check if such a type is a valid type using the 
boost::tti::valid_member_type metafunction.

With all the above said, allowing the end-user to specify whatever type 
he wants as his "invalid type" certainly seems easy enough to do.

>
> 5) The description of "Table 1.4. TTI Nested Type Macro Metafunction
> Existence" makes no sense to me (though it might later in the docs of
> course).

I will re-explain this better in the doc as the check for an "invalid 
type" when using BOOST_TTI_MEMBER_TYPE.

>
> 6) I found code formatting such as:
>
> boost::tti::has_static_member_data_DSMember
> <
> T,
> short
>  >
>
> A little hard to read, as long as the lines don't get too long I would
> much prefer:
>
> has_static_member_data_DSMember<T, short>

I can change this. The former, strangely enough, is my own preferred way 
of looking at template code. But I do understand I am adding many extra 
lines to the code formatting when I do this.

>
> 7) I'm not sure about the term "composite types": to me it's a member
> function pointer type, or if you prefer a function signature. So instead
> of BOOST_TTI_HAS_COMP_MEMBER_FUNCTION I'd prefer
> BOOST_TTI_HAS_MEMBER_FUNCTION_WITH_SIG.

I have no problem with that.

> In fact, I think the version
> with function-signature should be the main variant, so maybe
> BOOST_TTI_HAS_MEMBER_FUNCTION(IntFunction) would become:
>
> has_member_function_IntFunction<int (T::*)(short)>
>
> and some other name should be provided for the mpl::sequence version -
> if it's there at all?

I feel the mpl sequence version should be the main version. the reasons 
for that are as follows:

1) The mpl sequence versions follows the idea that the types in all the 
metafunctions should be broken down into individual types as much as 
possible. This is useful because one can then use the functionality in 
BOOST_TTI_MEMBER_TYPE to pass a nested type in any of the generated 
metafunctions, which may not actually exist, without producing a 
compiler error.

2) The reason for having the composite form of the member function ( or 
static member function ) at all was so one could enter a calling 
convention ( or possible some other compiler specific addition to member 
function or composite function syntax ) which the Boost function_types 
library does not cover through one of its tag types. Without this 
consideration I would have eliminated the composite type syntax 
completely, even if specifying it is syntactically easier than using the 
mpl sequence. I would have done this purely for the sake of keeping the 
idea of individual types as regular as possible. But since some 
compilers use strange calling conventions and occasionally other 
compiler-specific conventions when specifying function syntax I decided 
it was important to allow this composite syntax directly.

>
> I'm also thinking that what folks really want to know is: "Is there a
> function named X, that can be called with arguments of types A, B and
> C". It would be interesting to see how far along this road we could
> actually get?

I think Eric Niebler is the expert in this area regarding non-nested 
functions. But if he and others with more knowledge of determining this 
for free functions would be willing to help me I can certainly look at 
it in the future. I just do not think it belongs in this particular 
library as I conceive of it presently.

>
> 8) I confess I haven't done any metaprogramming lately, but the purpose
> of the sections "Macro Metafunctions as Metadata" and "Nullary Type
> Metafunctions" completely eluded me. So I'd like to know if this is just
> me ;-)
>
> My gut feeling at present is that the macros in these sections don't add
> much to the library and will likely confuse the heck out of new users,
> so I guess I'd like to see better descriptions and some killer use cases
> before accepting these parts.... but more opinions please!

The whole idea of this secondary set was to avoid manually having to 
drag out the ::type syntactically from the basic macro metafunctions 
when doing nested queries such as:

Does class X have a class Y which has a class Z which has a member 
function of signature A ? Once one gets into any sort of deeply nested 
query with TTI using the macro metafunctions involves quite a bit of 
::type syntactical notation and the nullary type metafunctions eliminate 
that for the end user.

Honestly I do think they serve a nice syntactical shortcut purpose and 
if they are confusing to you and others I would say to just ignore them. 
Of course I can remove them from the library without any loss of 
functionality, but I feel syntactical ease of use is worth it.

If there is a specific way, other than more examples, I can explain the 
nullary metafunction syntax better, I would love to hear it. I will add 
a section showing the synatctic difference using the macro metafunctions 
and the nullary type metafunctions, with more examples.

>
> 9) Great shame about the nested function template issue - hopefully
> someone can find a workaround.

Calling Daniel Walker ( or perhaps Dave Abrahams and Alexsey Gurtovoy ).

I more or less understand what the generated macros are doing but the 
issues of what it takes for a compiler to compile the code correctly are 
harder. There is a workaround which some earlier version of VC++ accepts 
so maybe it can be used with Intel 11.1 and 12.0. I will experiment. My 
own specific template match is heavily based on the MPL code so once a 
workaround can be found for Intel compiler for the MPL code, finding the 
same workaround for my specific template match code should be relatively 
easy.

>
> 10) Small point, but in the reference docs, for example for
> BOOST_TTI_TRAIT_HAS_COMP_MEMBER_FUNCTION(trait, name), it says:
>
> "returns = a metafunction called "boost::tti::trait" where 'trait' is
> the macro parameter."
>
> But it doesn't really return anything, rather it's a code generator. So
> I'd rather it said that it generates a metafunction:
>
> template<class T>
> struct trait
> {
> static const value = unspecified;
> typedef mpl::bool_<true-or-false> type;
> };
>
> Then go on to define T, value, and type (I assume there is a member
> named type??).

Agreed. 'Returns a metafunction' should always be 'generates a 
metafunction' with the appropriate metafunction explanation.

>
> 11) In the docs the main header file should be given as
> boost/tti/tti.hpp rather than just tti.hpp.

I will change that.

>
> 12) Testing BOOST_TTI_HAS_COMP_MEMBER_FUNCTION(begin)
>
> I see that this static assertion fails:
>
> BOOST_STATIC_ASSERT((boost::tti::has_comp_member_function_begin<std::vector<int>::const_iterator
> (std::vector<int>::*const)(void)>::value));
>
> Have I done something silly?

No, it is a bug. I am looking into it.

I thought I wrote some thorough tests but I now see I needed to test 
more. I did not write any tests for const member function or const 
static data, and that was poor of me.

> Testing const-member functions seems OK
> with BOOST_TTI_HAS_MEMBER_FUNCTION BTW.

Yes, this works. I have to figure out why one and not the other works 
and I will. Somehow the function types library is doing something very 
clever when composing a member function type which the direct syntax 
does not have.

Thanks for all the comments and taking the time to look at the library 
and try things out.

Eddie Diener