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: Frédéric Bron (frederic.bron_at_[hidden])
Date: 2011-07-15 13:20:00

> Please explicitly state in your review whether the library should be
> accepted.

YES, this library should be accepted after some issues are solved.

>    - 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 others that traits should be generated in the local
namespace instead of  boost::tti. Then the user has full control of
name clash.

>    - 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.
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&) { }

- 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 >).
   . 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? What if the enclosing type is itself a template
but not the inner type. In that case, the inner type is still a
template.

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

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

- typos:
   . in tti_detail.html: "will tell use" -> "will tell us" or "will
tell the user"
   . 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 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 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? By the way, I have seen that this is currently not tested as
all data and types are public in the tests (in structs).

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.

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.

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