|
|
Boost : |
From: David Abrahams (dave_at_[hidden])
Date: 2005-06-13 13:05:41
David Abrahams <dave_at_[hidden]> writes:
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.
---------
> 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
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk