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.
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).
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.
[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)
HAS_MEMBER_VARIABLE(has_myvar, [static] myvar)
HAS_MEMBER_FUNCTION(has_myfunc, [static] myfunc)
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.
[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.
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.
[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.
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)
)
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)
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
)
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
)
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)
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.
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.
11. [WANT] Is it possible to have some sort of composite syntax for
member variables? 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
13. [WANT] Are the nullary metafunctions really needed? 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.
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".
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...
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).
18. [NOTE] I don't think you need a space before "?" in English (see
"Why the TTI Library ?", etc).
19. [WANT] Typo in "depending on how many parameters are bring passed".
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?
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). I would also
comment on possible C++ standard extensions or compiler specific
support for introspection.
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
