Re: [boost] [function types][review]

From: David Abrahams (dave_at_[hidden])
Date: 2005-06-13 11:13:36

Just starting from the beginning:

This library contains .inl files.  In my opinion these should be .hpp
files, but if you insist on a different extension, please make them
.ipp.  Nico Josuttis made the argument for .cpp/.hpp in the early days
of Boost and it remains a good one: it's easier to find all the source
files with various tools, and you know which ones contain C++ source.

,----
| Use fewer words
|      
|      - E.B. White
`----

  Overview

  This library provides a metaprogramming facility...

I think you should drop the word “metaprogramming” here.  Lots of
people who don't think of themselves as doing metaprogramming might be
interested in this library and could easily be scared off.  Also, it
adds nothing to the essential meaning of the sentence

  For the purpose of this documentation, these types are collectively
  referred to as function types (this differs from the standard
  definition and redefines the term from a programmer's perspective to
  refer to the most common types that involve functions, however,
  classes with an overloaded parentheses operator are not considered a
  function type, here).

Put the parenthesized material in a footnote.  It's too long to be
part of that sentence and anyway it's too much detail for 99% of
readers, who won't care about the standard's definition.

   The classes introduced by this library shall conform to the
   concepts of the Boost Metaprogramming library (MPL).

This library introduces templates, not classes.

Drop the word “shall.”  It's unnatural (“shall” is normally used in
declarations of what will happen in the future) and adds nothing.
doesn't really conform to the way the C++ standard uses it, which is a
statement of requirements on what implementations of the standard must
do -- unless of course you think you're writing a standard that other
people will implement.

Also, you don't say which concepts. I would just say, “This library is
designed to work well with the Boost Metaprogramming library (MPL),”
or say earlier on “This is a library of metafunctions <link to MPL
concept definition>,” if that's accurate.  Yes, I know this somewhat
contradicts my admonition to drop “metaprogramming.”

 The Function Types library enables the user to:

    * test an arbitrary type for being a function type of specified
      kind,

“test whether a type is a specific kind of function type”

    * inspect properties of function types, such as function arity,
      result parameter types, etc.

Drop the 1st comma

    * view and modify sub types of an encapsulated function type with
      MPL Sequence operations, and

No space in “subtypes” (sub is not a word on its own -- repeated
throughout the document).  What is an “ecapsulated function type?”

    * synthesize function types. 

A tiny bit more detail about what this means should go here.  I am
guessing it means 

      function_ptr<R, A1, A2>::type  ==>  R(*)(A1,A2)

That would be “synthesize function types from argument and return
types.”

  Motivation

  Applying metaprogramming to increase the flexibility of software
  design often involves the inspection and manipulation of function
  types.

  A very common application is the automatic generation of callback
  facilities which occurs frequently in Boost libraries, such as Bind,
  Lambda, Phoenix and Function.

This is a very weak opening.  A person reading the Motivation section
wants to know, “what would I do with this library?”  I suggest
something like:

  Generic libraries that accept callable arguments are common in C++:
  STL, Boost.Lambda, Boost.Function, and Boost.Python are just a few
  examples.  Analysis and manipulation of built-in function types
  seems to appear in each one.

The above is by no means perfect, and it took me 5 minutes to write.
It can be hard to follow Mr. White's advice ;-)

   Other (often related) applications, include creating interface
   descriptions from function prototypes (extracting meta information
   about an interface). 

That's so redundant it's not worth saying.  All the periods in this
paragraph should be commas.

   concept checking (e.g. find out if two
   functions model a setter/getter pair). and systematic overload
   selection (creating a function pointer to e.g. std::sin requires a
   cast to either select the overload for float, double or long
   double).

Other applications include 

  - concept checking 

       [Show brief example asserting that two functions model a
       setter/getter pair]

  - systematic overload selection

       [Show brief example of how creating a function pointer to
       .g. std::sin requires a cast to either select the overload for
       float, double or long double]

  Currently Boost libraries redundantly use template partial
  specialization or overloading cascades to deal with function
  types. This has the following disadvantages:

Can't use “this” without an antecedent.  “This approach has the
following disadvantages:”
  
      * It is tedious to write when the library is implemented, 

It is tedious to implement.

      * most libraries fail to support functions with non-default
        calling convention or variadic parameter lists

      * proper support for the former point results in redundant work

Drop "point"

        for the authors, redundant per-library configuration settings

Need a comma right here.

        and redundant code being processed if these libraries are used
        in the same program.  

  This library gives developers a powerful tool at hand to avoid the
  above problems.

Drop "at hand."  But I'd probably drop the whole sentence.

  It covers the functionality of the function_traits class (introduced
  by the Boost Type Traits library), which has been agreed to be
  unsuitable for template metaprogramming in most situations, due to
  the fact that its interface encodes the index to function paramter
  types in identifier names and the inability to work with types other
  than plain non-member/static functions, which, from today's
  perspective, makes function_traits a weak spot of the otherwise
  excellent library. 

This sentence is too long!  I suggest:

  This library provides all the functionality of the function_traits
  <link> class template from the Type Traits library in a form that's
  more suitable for template metaprogramming.

If you like, add a link to a detailed explanation of the reasons that
function_traits is unsuitable, in a setting where you have the space
to write something comprehensible to non-gurus.  There's no room to
say anything useful about that in the introductory section of your docs.

  Other Type Traits classes covered as special cases by this library's
  is_function_type class are is_function and
  is_member_function_pointer.

"Also, is_function and is_member_function_pointer are covered as
special cases by this library's is_function_type class template."

---------------
  Interface

Some simple examples are missing right here.  In fact, I've gone well
past a whole page of reading with no examples.  I don't think we
should introduce any new libraries without a simple example in the 1st
page.  The rest of the doc has some examples, but only as linked
documents.  I haven't clicked through, but I imagine they're complete,
compiling programs.  That's too much information, too distantly
located, to be useful to the person trying to get a feel for the
library.  Note also that many people will be reluctant to click
through those because it will launch their IDE instead of displaying
in their browser.

---------------

  Synopsis:

All this stuff goes in namespace boost?  I don't think I'm happy about that.

   All names are actually defined inside local namespaces. They are
   injected to the boost namespace via a using directive.

Okay, that's a little better.  It avoids some ADL problems but still
brings many special-purpose names into the top-level namespace.  Do
the tag types belong there?  Also "local namespace" isn't a
well-accepted term.  How about "sub-namespaces of boost?"

I don't think the synopsis should show them in namespace boost and
then be amended by a footnote at the bottom that many people might not
read.  At least put a link to the note in the synopsis code.

    template<typename F>
    struct function_type_arity;

    template<typename F>
    struct function_type_result;

    template<typename F>
    struct function_type_class;

    template<typename F, typename I>
    struct function_type_parameter;

    template<typename F, long I>
    struct function_type_parameter_c;

    template<typename F>
    struct function_type_parameters;

Yowch!  I really hate to see a boost.whatever library with names like

        whatever_foo
        whatever_bar
        whatever_baz

in it.  Isn't this what namespaces are for?

The [...see Tag Types...] link just takes me to the top of the doc.
----------------

  Headers

  Header file names generally match the name of what their inclusion
  causes to be defined with the exception of
  function_type_parameter.hpp which also contains the class
  function_type_parameter_c.

"With one exception, each component is defined in a header file whose
name is the same as that of the component, with an .hpp suffix.  For
example, before using is_function_type, your functions should first
contain:

#include <boost/function_types/is_function_type.hpp

The exception to this rule is function_type_parameter_c, which is
defined in boost/function_types/function_type_parameter.hpp."

  The tag types are defined as soon as the first header containing a
  class that depends on them is included.

The headers don't contain classes.  

At this point, I have no clue what these dependencies are and can't
understand what this statement means.  All I can guess is that I don't
ever have to worry about specific #includes for the tag types.  If
that's the case, just tell me so :)

------

  Tag types

  As stated at the beginning of this document, there can be different
  kinds of function types: 

    # non-member/static function,
    # pointer to non-member/static function,
    # reference to non-member/static function,
    # member function pointer,
    # member function pointers that are const-volatile qualified,
    # each of the above with a parameter list ending with an ellipsis operator, and
    # each of the above for every calling convention defined by the C++ implementation in use.

  The Function Types library uses tag types to describe differnt kinds
  of function types, and arbitrary supersets of these and their
  inverse. [Show an example of a simple tag type in use]

You can't represent "arbitrary supersets;" you only represent the
supersets that you've chosen to support.  

"Each kind of function type has a corresponding tag type:

      * non-member/static functions:               plain_function 
      * pointers to non-member/static functions:   function_pointer  
      * references to non-member/static functions: function_reference
      ...etc...

There are also tags to represent groups of function types:

      * Any function type at all:       any_function
      ...etc..."

The document doesn't say anywhere what no_function is.

   It has no relevance to the user, how these tags are implemented

Drop the comma.

   except that they exist and that they are types that can be passed
   as template type arguments.

--------

Classification

I always learned that it is bad form to have a section with only one
subsection.  You have several of these.

I would list all of the metafunctions except for the decomposition
metafunctions at the top level and have a section called
"Decomposition Metafunctions".  Except there seems to be something
missing alongside is_function_type:  I want a metafunction that
returns the tag corresponding to a given function type's kind.

---- 

  is_function_type

  Members

  value	- 	Static constant of type bool, evaluating to true if T is

Uhm, the essential member of any metafunction is ::type, and I don't
see it listed here.  Value doesn't "evaluate to true", it "is true."

  an element of the set of types specified by Tag. Member function
  pointers may be more const-volatile qualified than specified,

Hmm, have you looked carefully at the use cases for this?  Member
functions often have counterintuitive is-a relationships.  For
example, a base class member pointer is-a derived class member
pointer, but not vice-versa.  It's not obvious to me that any is-a
relationships should hold here.  What are the use cases?

Need "and" right here.

  top-level const-volatile qualification of pointers is always
  ignored. Unless explicitly specified otherwise by the Tag, both

"Unless the tag explicitly indicates otherwise, ..."

  variadic and non-variadic function types of any calling conventions

"convention"

  are matched.

  ...	- 	See the MPL Integral Constant concept.

What??  What kind of member is "..."??  This is unacceptably vague.

  Header
              <boost/function_types/is_function_type.hpp>

  Member function pointers may be more const-volatile qualified than
  specified

---------

  Decomposition

  Six class templates are provided to extract properties of a function
  type, such as arity, parameter-, class (where appropriate)- and
  result type.

"Six class templates are provided to extract properties of a function
type, such as arity and the types of the result, parameters, and the
class targets of member functions."

-------

   template<typename T>
   struct function_type_arity;

  Template parameters

  T - The function type to be inspected. T must be a function type as
      defined at the beginning of this document
      (is_function_type<no_function,T>::value == false).

You shouldn't repeat that verbose description all over the place.

  "T - Any function type <link to definition>"

is better and more concise.

  Members

  value	- 	Static constant of type size_t, evaluating to the number
  of parameters taken by the type of function T describes. The hidden
  this-parameter of member functions is never counted. 

That particular behavior does not match the needs of any use cases
I've seen.  I always want to treat the hidden this parameter as the
function's first parameter (actually as a reference to the class, with
cv-qualification given by that of the member function itself).  What's
the rationale?

  If T describes a variadic function prototype (e.g. printf) the
  ellipsis parameter is not counted.

--------

   template<typename T, typename I>
   struct function_type_parameter;

  Template parameters

  ...

  I	- 	The index to select the parameter type to be returned (MPL
  Integral Constant). Valid indices start with zero (for the first
  parameter). 

Putting "MPL Integral Constant" in parens has no agreed-upon meaning
and seems arbitrary and needlessly terse.

  "I - An MPL Integral Constant describing the index of the parameter
  type to be returned. The index of the first parameter is zero."

What about the hidden "this" parameter?  I think you know the
semantics I want, but you don't say what you provide.

--------

  Encapsulation

  While the decomposition part of the interface provides a fine

Need a hyphen here.

  grained set of traits classes to find out distinct aspects of a
                                   ^^^^^^^^
"discover," or "query"

  function type, specializations of the class template
  function_type_signature in contrast represent a function type in
  order to work with it as a whole.

  Therefore function_type_signature can be used with MPL Sequence
  operations to work with the representee type's sub types.
                              ^^^^^^^^^^^
"represented"

This begins to sound like function_type_signature<T> is an MPL
sequence.  I would just drop the sentence.  Oh, wait, after reading
the whole description I now believe it is a Random Access
Sequence... but you never tell me what the sequence contains!!  This
is a serious omission.

   Other properties, such as arity, kind of function type and the
   representee type, are contained as type members. So
   ^^^^^^^^^^^
"represented".  Turn the period into a comma.

   function_type_signature  can also be used as a traits
   bundle. However, this class template should be seen as a white box,

I don't know what that means.  Please use a more generally understood
term, or just drop that part of the sentence.

   so using the primary interface for clasification and decomposition

Drop "using"; you repeat "use" below.

   is recommended for general use. 

I don't know what you mean by this recommendation.  If I have a
specialization of function_type_signature, how am I supposed to get
information about it, or use it?  Am I supposed to pass it to the
decomposition metafunctions (I don't think so!)?

----
BTW, the use of "Ts" as a parameter is confusing.  I keep reading the
"s" as a subscript.  It's not a list of Ts that's being passed; it's a
list of "Types."  Why not just use "Types?"
---
   template<typename Tag, typename Ts>
   struct function_type;
Needs a brief description; maybe all do.  Example: "Returns a new
function type built from a sequence of types."
  Template parameters
  Tag	- 	Tag type specifying the kind of function type to be
  created. Abstract tag types (ones that describe abstract supersets
  of types such as any_function) are not allowed with the exception of
  no_function (see below for effect). 	
"Tag types that describe abstract supersets of types, such as
any_function, are not allowed with the exception of no_function (see
below<link>)."
  Ts	- 	Model of MPL Forward Sequence of sub types, starting with
Drop "Model of"
  the result type optionally followed by the class type (if Tag
  specifies a member function pointer type) 
That sounds like if Tag specifies a member function pointer you have
the option to represent the class type.  Just drop "optionally" and
remove the parens.
So here you are using the form I like, where class types are treated
just like any other (should be a reference, though).  Why not do this
uniformly.
  followed by the parameter
  types, if any. If Ts is not a sequence (mpl::is_sequence<T>::value
  == false), a specialization of function_type_signature for this type
  is used. 
This is unintelligible.  You already said Ts is required to be a
sequence.  Now you're taking it back? A specialization of
function_type_signature for *which* type is used?  Used how?
  This may fail if function_type_signature is an incomplete
  template due to forward-declaration without definition.
Why would that ever be the case?  Shouldn't your library take care of
it?
  Members
  type	- 	The function type as specified by the template
  arguments. 
"The synthesized function type"
  Unless explicitly specified otherwise by Tag, type is a
  non-variadic function of default calling 
"convention"
  . If Tag is no_function,
  type is identical to Ts if Ts is an MPL Sequence other than a
  specialization of function_type_signature, in which case an type is
  an mpl::vector (this is primarily for internal use of the library
  but, since it may be useful in some situations, there is no reason
  to hide it from the user).
Oh yes there is!!  I can't for the life of me understand these
semantics.  Guideline: anything whose specification is too complicated
to explain easily probably needs to be redesigned.
Also, special-case "if" clauses, especially those that test whether
something matches a concept (like "is_sequence") tend to destroy
genericity.
----
  Portability
  Currently this library does not contain any workarounds for broken
  compilers. It was successfully tested with Microsoft Visual C++
  Versions 7.1 to 8.0, Intel Linux Versions 7.0 to 8.1 and GCC Version
  3.2 to 4.0. The library is expected to work with compiler using the
  Edison Design Group frontend other than Intel, although currently
  there is no proof for this.
Okay, I appreciate the desire to avoid broken compiler workarounds.
However, many of the libraries in Boost that could use this library do
support broken compilers, and therefore -- I think -- are not using
idioms that can readily leverage metafunctions for this purpose.  The
true measure of whether this library should be accepted, IMO, is in
whether it can be used to eliminate large amounts of code from other
libs.  Can it?  The quickest way to a proof-of-concept is to apply it.
Incidentally, I think
http://lists.boost.org/MailArchives/boost/msg81758.php makes it
possible to implement almost any trait we previously thought was
impossible in pre-7.1 Visual C++.
I'm not ready to accept this library, and I'm not ready to reject it
either.  It has the potential to be extraordinarily useful, but that's
unproven as far as I'm concerned.  I have some substantial complaints
with the interface, and I have some serious problems iwth the docs as
you can see (not all of those remarks are trivial grammatical edits!)
-- 
Dave Abrahams
Boost Consulting
www.boost-consulting.com