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

Subject: Re: [Boost-users] [boost] [Review] Type Traits Introspection library by Edward Diener starts tomorrow Friday 1
From: Edward Diener (eldiener_at_[hidden])
Date: 2011-07-15 18:07:09

On 7/15/2011 1:20 PM, Frédéric Bron wrote:
>> Please explicitly state in your review whether the library should be
>> accepted.
>
> YES, this library should be accepted after some issues are solved.

Appreciated !

>
>>     - What is your evaluation of the design?
>
> The names of include files are not easy to remember because they are
> not fully conformant to the macro name (sometimes yes, sometimes not):
> - HAS_TYPE is in type.hpp
> - HAS_TEMPLATE is in template.hpp
> - HAS_TEMPLATE_CHECK_PARAMS is in template_params.hpp ->
> template_check_params.hpp?
> - HAS_MEMBER_DATA is in mem_data.hpp ->  member_data.hpp?
> - HAS_MEMBER_FUNCTION is in mem_fun.hpp ->  member_function.hpp?
>
> I would use the lower case version of the macro with BOOST_TTI_HAS_
> removed so that it is easy to remember.

I agree with this also and will change the names of the header files 
accordingly.

> I agree with others that traits should be generated in the local
> namespace instead of  boost::tti. Then the user has full control of
> name clash.

I will be removing the generated macro metafunctions from the boost::tti 
namespace and not putting them in any namespace. I have been duly 
chastised for my short-sightedness <g>.

>
>>     - What is your evaluation of the implementation?
>
> I have not been in detail in the implementation.
> Just opening some files and this is what came out: can you remove
> trailing spaces?
>
>>     - What is your evaluation of the documentation?
>
> - simple examples should appear very early in the doc. You could maybe
> add a tutorial section starting with the simplest examples, leaving
> the more complex to the end.

My intention for the documentation is to change it so that each macro 
metafunction gets its own topic with a fuller explanation and a set of 
examples starting with the simplest to the more complex.

> You should also give an application example of the trait itself.
>
> for example (untested):
> template<class T, bool Has_a=has_member_data<a, int>::value>
> void print(const T&);
> template<class T>
> void print<T, true>(const T&t) { std::cout<<t.a<<'\n'; }
> template<class T>
> void print<T, false>(const T&) { }

I can understand the need for practical application examples in the 
documentation. The problem, as I see it, is that whatever application 
examples I give others will find much better practical uses. Perhaps it 
would be better just adding an examples subdirectory below libs and 
creating some examples which are actual small applications, with enough 
documentation in the source files of these examples so that the end-user 
could get an idea of how TTI may be used. For instance I notice that 
type traits has some basic example programs and I can create some for 
TTI and then just inline the code in the documentation the way type 
traits does it.

>
> - Table 1.2.
>     . TTI Macro Metafunctions, it is not clear what means "Type with check".
>     . You could add<  T>  or<  T, U>  after the trait name (for example
> boost::tti::has_type_'name'<  T>).

The dodcumentation does say:

class T = enclosing type
class U = type to check against

and then clicking on the metafunction names takes you to the reference 
for that metafunction.

But as I said above I will give a more extensive explanation with 
examples for each metfunction, with some examples.

>     . BOOST_TTI_HAS_TEMPLATE, BOOST_TTI_VM_HAS_TEMPLATE_CHECK_PARAMS
> and BOOST_TTI_HAS_TEMPLATE_CHECK_PARAMS: not clear from the table what
> it checks for. Is there a class inside T that is declared templaste<
> typename...>  name?

Click on the metafunction name for more info.

BOOST_TTI_HAS_TEMPLATE just checks for a template 'name' inside T with 
all class/typename template parameters ( from 1 to 5 template parameters 
currently ).

BOOST_TTI_VM_HAS_TEMPLATE_CHECK_PARAMS and 
BOOST_TTI_HAS_TEMPLATE_CHECK_PARAMS check for an exact match of a 
template's parameters of a specific 'name' inside T. The first uses 
variadic macro syntax to give the exact template parameters while the 
second uses a pp-seq to give the exact template parameters.

All three produce a metafunction which takes an enclosing type and 
returns a true or false value.

In an extensive discussion with others I have decide to subsequently 
change these macro metafunctions to a single BOOST_TTI_HAS_TEMPLATE, 
which will work similarly but are syntactically different depending on 
whether variadic macros are supported.

> What if the enclosing type is itself a template
> but not the inner type. In that case, the inner type is still a
> template.

All enclosing 'types' are types. Templates themselves are not 
introspected. But a type's inner template may be found through 
introspection.

>
> - the documentation of for each macro is too succinct. You should
> re-explain the naming convention that will be used by the macro to
> create the new trait. You could even just give the C++ code that will
> be produced by the macro (just the visible code):
>
> BOOST_TTI_HAS_TYPE(name)
> is equivalent to:
> template<  typename T, typename U=notype>
> struct has_type_name {
>     const bool value;
> };
> with value ...
>
> In particular, examples are required for each of them.

I hear you. I really don't want to duplicate the source code for each in 
the doc. Some are pretty long and involved.

>
> - examples in tti_usingNTM.html
>
> In general, I think simple examples miss. Complex examples are given
> but simple not.
> For example, for has_type, I quote:
> "Does T have a nested type called 'DType' within 'BType::CType' ?
> BOOST_TTI_HAS_TYPE(DType)
> boost::tti::mf_has_type
>    <
>    boost::tti::has_type_DType<_>,
>    CTypeNM
>    >
> We could just have easily used the boost::tti::mf_valid_member_type
> metafunction to the same effect:
> boost::tti::mf_valid_member_type
>    <
>    DTypeNM
>    >"

I will give simpler examples first, then more complex. As mentioned 
above, each macro metafunction will get its own topic with individual 
examples which explains the metafunction usage more clearly.

>
> I would be happy to see simple examples like the following:
> BOOST_TTI_HAS_TYPE(B)
> BOOST_TTI_HAS_TYPE(mytrait, B)
>
> struct A {
>     struct B { }
> };
>
> boost::tti::has_type_B<A>::value; // true
> boost::tti::has_type_B<int>::value; // false
> boost::tti::has_type_B<B>::value; // false
>
> boost::tti::mytrait<A>::value; // true
> boost::tti::mytrait<int>::value; // false
> boost::tti::mytrait<B>::value; // false

Great idea ! I will definitely do that and follow that format for the 
simpler examples.

>
> - typos:
>     . in tti_detail.html: "will tell use" ->  "will tell us" or "will
> tell the user"

Corrected !

>     . in tti_nested_type.html: "The goal of the TTI library is never to
> produce a compiler" ->  "The goal of the TTI library is to never
> produce a compiler"

I do not think that either variation is more correct than the other, but 
this is one area of English grammar about which I admit I am unsure.

>
> - I have seen no explanation of the implementation behavior with
> regards to public/protected/private category of inner type/member.
> Could give details in the documentation?

I actually forgot about this completely when I created tests. I 
definitely fell asleep in this area. Ideally one should be able to 
introspect only public inner elements successfully, since one is 
introspecting from outside the class, while protected and private 
element introspection should return false.

Another reviewer brought this up and I realized that TTI might benefit 
from the ability to introspect protected inner elements and private 
inner elements. I have thought about ways to do this, aided by other 
code I have seen, and I have some plans, but I have delayed trying them 
out until the review was over and I could work on the changes suggested 
by others.

> I imagine there is no need to
> introspect private/protected types/members. But what happens if the
> type/member exists but is private? Compilation error or value==false
> returned?

Ideally it should never be a compiler error, but just return 'false'. In 
reality I may not be able to guarantee that. So further below for a 
brief discussion in my response.

> By the way, I have seen that this is currently not tested as
> all data and types are public in the tests (in structs).

This is where I nodded <g>. Furthermore my tests did not really test 
const/volatile cases but I think this is much less of an eventual 
problem. I really must expand my tests.

>
> I have tested the following program:
> #include<iostream>
>
> #include<boost/tti/type.hpp>
> #include<boost/tti/mem_data.hpp>
>
> class A {
>          private:
>                  class private_type { };
>                  int private_data;
>          protected:
>                  class protected_type { };
>                  int protected_data;
>          public:
>                  class public_type { };
>                  int public_data;
> };
>
> BOOST_TTI_HAS_TYPE(private_type)
> BOOST_TTI_HAS_TYPE(protected_type)
> BOOST_TTI_HAS_TYPE(public_type)
> BOOST_TTI_HAS_MEMBER_DATA(private_data)
> BOOST_TTI_HAS_MEMBER_DATA(protected_data)
> BOOST_TTI_HAS_MEMBER_DATA(public_data)
>
> int main() {
>          std::cout<<boost::tti::has_type_private_type<A>::value<<'\n'; // true
>          std::cout<<boost::tti::has_type_protected_type<A>::value<<'\n'; // true
>          std::cout<<boost::tti::has_type_public_type<A>::value<<'\n'; // true
>          std::cout<<boost::tti::has_member_data_private_data<A,
> int>::value<<'\n'; // compile time error
>          std::cout<<boost::tti::has_member_data_protected_data<A,
> int>::value<<'\n'; // compile time error
>          std::cout<<boost::tti::has_member_data_public_data<A,
> int>::value<<'\n'; // true
>          return 0;
> }
>
> has_type answers true for private, protected and public member types
> but has_member_data fails to compile if the data member is private of
> protected (tested with g++ 4.4.4 (log attached).
>
> I do not know if this is possible or not but I think the behavior should be:
> - private or protected ->  value==false even if the type/member exists
> - public ->  value==true
> because private and protected are implementation details which should
> be ignored.

I totally agree with what you feel the results should be. Now to some 
explanations.

I am not sure offhand why the protected/private types are found but I 
will look at it. The code is really the MPL code for 
BOOST_MPL_HAS_XXX_TRAIT_DEF/BOOST_MPL_HAS_XXX_TRAIT_NAMED_DEF which I am 
just reusing in TTI. I will look at that to see if I can tweak it 
accordingly.

I know why the protected/private members give a compiler error and there 
may not be a way around it, but I will certainly try to find one. The 
problem is that SFINAE is used to determine if the address of the member 
can be taken ( the same goes for member functions, static data, and 
static data members ). According to current SFINAE, but not as I 
understand it to extended SFINAE in C++0x, the data can have its address 
taken without it being considered a SFINAE failure even if the data is 
not accessible. Then of course once that member is chosen as valid, an 
access error occurs because it is not accessible.

As I understand it with extended SFINAE in C++0x, extended SFINAE does 
kick in to rule out the member function if it is not accessible, 
therefore giving not a compiler error, but a false condition, which is 
what we want. However the number of compilers supporting C++0x SFINAE 
are probably small right now.

>
> For example what is the need to know if member m of type double
> exists? probably to use m somewhere. But if m is private, the fact
> that m exists does not help very much.

I agree. I will work on this area to correct it.

>
>>     - What is your evaluation of the potential usefulness of the
>>       library?
>
> I am sure that it can be usefull to implement the best algorithms to
> appropriate types.
> For example, you can imagine that adding an element at the front of a
> container could be specialized for containers that have member
> function push_front by using it and for other containers by using more
> complex copying algorithm. Then the best algorithm is used
> tranparently to the final user.
>
>>     - Did you try to use the library?  With what compiler?  Did you
>>       have any problems?
>
> Yes, I tried to use the library with g++ 4.4.4. It worked as expected
> apart for private members as reported (tested HAS_TYPE and
> HAS_MEMBER_DATA).
>
> I ran tests with intel 11 and 12 which failed but the author is
> dealing with this.

It is really an MPL problem too with Intel 11/12, which I reported and 
assigned to Joel Falcou. I will help with it as much as possible.

The Sun C++ compiler also has the same problem, along with a few more, 
which I will be looking at.

> I ran tests with g++ 4.4.4 and nothing failed (tested tti target only).
>
>>     - How much effort did you put into your evaluation? A glance? A
>>       quick reading? In-depth study?
>
> I read the doc + some testing (about 4 hours).
>
>>     - Are you knowledgeable about the problem domain?
>
> I am a bit aware of metaprogramming as I am trying to add an extension
> to the type trait library for detecting existence of operators.
>
>> And finally, every review should answer this question:
>>
>>     - Do you think the library should be accepted as a Boost library?
>
> YES.
>
> Frédéric

Thanks very much for the review, the suggestions, and finding the 
private/protected problems, even if I belatedly realized what most of 
them are from some earlier remarks about the library by others.

Eddie Diener