Re: [boost] Function Types Review Begins Monday, November 6, 2006

From: Tobias Schwinger (tschwinger_at_[hidden])
Date: 2006-11-09 13:46:51

Hi Richard,

Richard Smith wrote:
> 
> Throughout these comments, I am assuming the namespace alias
> 
>   namespace bft = boost::function_types;
> 
> 
> 
> 1.  Namespace
> 
>   The public interface to this library is in the
>   boost::function_types namespace.  As a propsective user of
>   the library, it feels very much like an extension to the
>   type_traits library, and that leaves me wondering why the
>   library isn't in the main boost namespace with the other
>   type_traits functionality.
> 

Well, we can probably pull some of the templates down to boost (see below) in case the library gets accepted into Boost.

> 
> 2.  Use of Tags
> 
>   After a second glance, it's clear that one reason for
>   having separate namespaces is to avoid name collisions --
>   we already have a boost::is_function, for example.  And
>   this leaves me wondering how boost::is_function differs
>   from bft::is_function as the names fail to convey the
>   distinction.  So far as I can see, the additional Tag
>   template parameter is the only difference, and only three
>   of the possible property tags are meaningful: variadic,
>   non_variadic and default_cc.

No. In fact is_function is a candidate to be put in namespace boost.
The tag parameter has a default, so boost::is_function and bft::is_function are compatible.

> 
>   The majority of the time I can't imagine myself making
>   much use of any of these tags.  Are they really worth
>   while?  The 'Rationale' section of the documentation
>   addresses this and considers some alternatives:
> 
<snip quote>
> 
>   What about the third possibility: leave it up to the
>   client to sythesise these more complicated examples.  It
>   will involve adding a few more templates: notably
>   is_variadic and is_default_cc.  Then, instead of writing
> 
>     bft::is_function< T, bft::variadic >::value
> 
>   you would write
> 
>     boost:is_function<T>::value && boost::is_variadic<T>::value
> 
>   ... which is not significantly more verbose.  I think this
>   would be my prefered interface as it would avoid the
>   duplication of functionality between the type_traits
>   library (and the potential for confusion that this would
>   cause).
> 

I think, I don't like it. It's too irregular for my taste.

> 
> 3.  Extensibility
> 
>   The internal bitmask implementation underlying the
>   property tags does not appear to allow extension by a user.  

Hell - don't mess with the bitmask - it's a library internal ;-) !

>   For example, there is a default_cc tag; suppose I
>   want to add support for querying the some other calling
>   convention.  

You can.

> Can I write a fastcall_cc tag?  

Even better: It's written for you. 

> Scanning
>   through the implementation it would appear not --
>   certainly there's no documentation on how to do so.

All you have to do is set the macros BOOST_FT_CC_NAMES and BOOST_FT_CC_<name> 
appropriately (see Reference->Macros).

> 
>   If a tag-like syntax is considered desireable, wouldn't
>   something like
> 
>     template < typename T,
>                template <typename> class UnaryTypeTrait >
>     struct is_function
>       : integral_constant< bool,
>           is_function<T>::value && UnaryTypeTrait<T>::value
>         >
>     {};
> 
>   be more extensible?  Or does this have to work with
>   compilers that can't handle template template parameters?

No. But it's supposed to work with MPL-Lambda Expression, which do not allow template template parameters.

> 
> 
> 4.  Naming
> 
>   It seems unlikely that the default_cc tag will be
>   particularly heavily used.  Given this, why not give it a
>   more meaningful name: default_calling_convention perhaps?

For the default configuration all calling convention tags currently have a _cc suffix.
But we might as well s/_cc/_calling_convention/g.

> 
> 
> 5.  Extern "C" linkage
> 

<snip>

> (mostly because you can't put extern "C"
>   within a template or atemplate within an extern "C"
>   block).
> 

Exactly.

> 
> 6.  What is a 'callable builtin'?
> 
>   So far as I can see, the C++ Standard does not define
>   'callable', so I asume that the pertinent definition
>   is the one in TR1, 3.1/6:
> 
>   | A /callable type/ is a pointer to function, a pointer to
>   | member function, a pointer to member data, or a class
>   | type whose objects can appear immediately to the left of
>   | a function call operator.
> 
>   By this definition, function types are not callable, nor
>   are references to functions, bizarre as this may seem.
> 

<snip>

> 
>   Though I'm not wholly convinced with TR1's definition of
>   'callable', this is as close as it gets to a standard
>   definition, and it would seem wise to make pointers to
>   member data return true.
> 
>   I also think 'builtin' needs documenting even if only
>   superficially; to me 'builtin' is a synonym to
>   'fundamental', though there are no callable fundamental
>   types.
> 

Obviously these terms do not work well... We need a definition or we have to find a better way of naming the beast.

> 
> 7.  What is a 'callable scalar'?
> 
>   The term 'scalar' is defined in 3.9/10:
> 
>   | Arithmentic types, enumeration types, pointer types, and
>   | pointer to member types, and cv-qualified versions of
>   | these types are collectively called scalar types.
> 
>   Note that references, whether to functions or otherwise,
>   are *not* scalars.  The is_callable_scalar metafunction
>   returns true for references to functions.  This is wrong.

Doh!
 
>   Also, why is is_callable_scalar useful?  I'm not sure I
>   can think of a use case; and if I could, I imagine it
>   would be rare enough that it wouldn't be much of an
>   inconvenience to sythesise it from is_function and
>   is_callable_builtin.
> 

Good point. Probably we can just throw it out. 

> 
> 8.  'Tag Types' documentation
> 
>   In several places, the 'Tag Types' documentation
>   incorrectly gives types two leading underscores.
> 

Where?!
 
> 9.  Extending result_type
> 
>   Perhaps this is beyond the intended scope of the library,

Yes.

>   but I think it would be very useful to extend the domain
>   of this metafunction to include class types with a
>   result_type member type. 

No, that's what boost::result_of is for. FunctionTypes is only about the type system. One of its examples is a reimplementation of boost::result_of, in fact.

<snip>

> 
> 10.  Syntactic sugar around parameter_types
> 
>   Whilst being able to leverage the full power of the MPL
>   with parameter_types is definitely desireable, it can also
>   faze those less familiar with metaprogramming.
> 
>   I would like to suggest a helper metafunction that
>   extracts a single parameter type:
> 
>     template < typename F, std::size_t N,
>                class ClassTransform = add_reference<_> >
>     struct function_parameter {
>       typedef typedef mpl::at<
>         function_parameters<F, ClassTransform>, N
>       >::type type;
>     };
> 
>   (I'd suggest base-zero numbering as given here, though
>   this might cause confusion in conjunction with the
>   first_argument_type, second_argument_type typedefs in
>   std::binary_function.)

Good practice is to typedef the sequence and use mpl::at_c on the new name.
It also leads to more readable code, IMO. 

Further, I think it's a good idea to encourage people to use the MPL.

> 
>   Oh, and I'd also like to suggest a quick comment in the
>   documentation saying what the '_' is that is given as a
>   template argument to add_reference -- a quick
> 
>     using mpl::placeholders::_;
> 
>   in the synopsis would probably be sufficient.

I'll add that.

 
> 11.  Member function arities
> 
>   The documentation to function_arity states that the 'this'
>   pointer is included -- which is what most people would
>   expect.  It's also what the implementation does.  However
>   in the 'Use Cases' page of documentation, a R (C::*)()
>   function is listed in the first code fragment under the
>   comment '0 parameters'.  Although not explicitly
>   contradictory (parameters and arity don't *necessarily*
>   mean the same thing), I think this is potentially
>   confusing.

I'm afraid that making the same point while addressing this issue could be more confusing. Any suggestions?

> 
> 12.  Use cases documentation
> 
>   To be honest, I find this page of the documentation
>   dreadful, which is shame as the rest is quite reasonable.
>   It discusses various scenarios that could benefit from the
>   application of this library, but utterly fails to suggest
>   *how* they would benefit from the library.
> 
>   On the whole page, only one of the library's components is
>   actually mentioned -- the function_pointer metafunction
>   for synthesising function types.  But the code fragment is
>   far too incomplete to be of any real use.  It's also one
>   of the more complicated examples as it requires using the
>   MPL.  By all means include such a example, but not as the
>   only one!
> 

Did you follow the links? There's a lot of inline documentation in the files.
But maybe I don't get your point.

Further, as mentioned above, I'm all against MPL-fear-mongering.

> 
> 13.  Why is bft::components useful?
> 
>   For that matter, what *exactly* does it do?  

It gives a one-type respresentation of all properties. And IIRC there is at least one example that shows it in action.

> First the
>   documentation says
> 
>   | components<T,ClassTransform>
>   |   MPL - Front / Back Extensible Random Access Sequence
>   |   of all component types and property tag
> 
>   then it says
> 
>   | Extracts all properties of a callable builtin type, that
>   | is the result type, followed by the parameter types
>   | (including the type of this for member function
>   | pointers).
> 
>   The former is ambiguous -- it doesn't say what it means by
>   'component types'; > and the latter contradicts it by not
>   mentioning the property tag.
> 
>   It also seems not to mention what it actually means by
>   *the* property tag.  A function can be variadic or
>   non_variadic; it be default_cc; it may have cv-qualifiers,
>   which in the case of a const volatile function needs to
>   tags to express it.  Are these concatenated using bft::tag
>   or appended one-by-one to the end of the MPL sequence?

The expression models two concepts.

> 
>   But more importantly, why do we need this metafunction?
>   It does nothing that separate invokations of result_type,
>   parameter_types and the various is_* metafunctions can't
>   achieve.  And I would *rather* see separate invokations of
>   the these -- it would make for much more readble code.
> 
>   Oh, and the name isn't very descriptive.

Well, read it like

   function_type's::components

. I've seen names worse than that...

> 
> 
> 14.  Type sythesis
> 
>   ... And now I see what bft::components is supposed to
>   achieve.
> 
>   But I still don't find the idea of merging the result
>   type, parameter types and arbitrary other information
>   together into a single type list at all intuitive.
> 
>   What's wrong with:
> 
>     template < typename Result, class ParameterSeq,
>                class PropertiesTag = null_tag >
>     struct function_type;
> 
>   Much more readable, in my opinion.
> 

Maybe. But I'm not sure. 

>   Having said that, I think these metafunctions are an
>   important piece of functionality.  It's merely their
>   interface I'm concerned about.
> 
> 
> I hope these points have been useful -- and sorry for such a
> long post if they haven't!
> 

Nothing to be sorry about. Thank you for your review!

Regards,

Tobias