Re: [boost] Fomal review period for functional

From: Jens Maurer (Jens.Maurer_at_[hidden])
Date: 2000-06-16 15:49:49

I think the library is useful.  The "references to references"
problem bit me a few months ago as well, and it's a pain.
Likewise, the library adds missing function traits classes similar
to iterator_traits, which fixes an asymmetry.

I vote for approval into boost.

Here are some detailed comments:

Documentation
-------------

General: Is there any boost preference which English language
variation to use (US -ize vs. British -ise etc.)?

index.htm: I would add an express reference to section 20.3
  [lib.function.objects] of the C++ standard for the uninitiated.

  There's a slight HTML buglet at the end of the "Contents" section
  ( "/li>" shows ).

  I would find it helpful to have a list of compilers with which this
  library is known to work.  In particular, missing partial specialization
  seems to leave use of "ptr_fun" required.

  I would add an explanatory note that libraries allowing LISP-
  like lambda-expressions with a more natural syntax are currently
  under development, but these may not work with certain popular
  compilers.

  Is there any reason why this file is called "index.htm", but the
  rest has the (IMHO proper) ".html" suffix?

function_traits.html: "... slightly different due [to] constraints"
  I'm not a native speaker, but isn't the "to" is required?

  The documentation should list all the relevant typedef's
  of the two traits classes in a concise "Synopsis" or
  somesuch.  Not an "example" and not an incomplete listing
  which is later augmented by "Other Types Defined".  I would
  also prefer an abstract description of each typedef such as
  "result_type is the return type of the Operation functional
  given as a template parameter" (please adjust wording to taste)
  before delving into "Usage" examples.  This abstract
  description should also encompass the additional "function_type"
  and "param_type" typedef's.

  The description of "function_type" has an incomplete sentence
  at the end: "If this typedef was not provided, ???".   For
  additional English beauty, try "If this typedef were not
  provided, ...".

  Please add the BOOST_NO_TEMPLATE_PARTIAL_SPECIALIZATION
  limitation to the "Limitations" section.

negators.html: Please add an express reference to std:20.3.5
  [lib.negators]. 

  The example has the template argument(s) of std::vector
  eaten as HTML markup.  Use < and friends to be happy.

  I would not hide the explanation why your re-implemenation
  is better in the "Usage" section, but put it right on top
  after the "not1, not2" listing.

  In section "Argument Passing", "Accordingly, the negators in this
  library (mem_fun1_t and its variants)...".  It is news to
  me that "mem_fun1_t" is a negator.

  I find the "Argument passing" section not very easy to
  understand.  I would suggest a "Synopsis" section with
  complete declarations so that the use of call_traits::param_type
  becomes clear.

binders.html: Please add an express reference to std:20.3.6
  [lib.binders]. 

  The example has the template argument(s) of std::vector
  eaten as HTML markup.  Use < and friends to be happy.

  I would suggest a "Synopsis" section with complete declarations.

mem_fun.html: Please add an express reference to std:20.3.8
  [lib.member.pointer.adaptors].

  I would augment "The first_argument_type typedef has been
  corrected" with "for the const_* member function adapters"
  so that things are put into perspective.

  The "first_argument_type" section is nice.

  I don't think that a "Synopsis" section would be good here,
  because it would contain too many tedious repetitions.

  I would rework the first part of "Argument Passing", because
  it presents the tool (for the solution of the problem) before
  the problem itself.

ptr_fun.html: Please add an express reference to std:20.3.7
  [lib.function.pointer.adaptors].

  In the example, I don't see why using boost::ptr_fun instead
  of std::ptr_fun provides any advantage.

  I would suggest a "Synopsis" section with complete declarations
  so that the enhancement is clearly visible.

  The "Argument Passing" section is somewhat difficult to
  understand.

  "Accordingly, the function pointer adapters in this library
  (mem_fun1_t and its variants) ..."  It is news to me that
  "mem_fun1_t" is a function pointer adapter.

Implementation
--------------

functional.hpp: I would add a comment to the namespace-closing
  braces, like so:   " } // namespace detail ".  These braces
  are among the most farthest away from their opening counterpart,
  so a little help for the code reader might be nice.
  Same for the include guard "#endif".

  I would add the "BCC workaround" comment to not2 and everywhere
  else it applies as well.

function_test.cpp: Is there any reason why the 
  #ifndef BOOST_NO_TEMPLATE_PARTIAL_SPECIALIZATION is repeated
  multiple times instead of excluding larger codeblocks?

  You seem to have a  std::cout << "\n"  before every output.
  Wouldn't it be better to have it after?  In particular,
  that would add the missing newline at the end of the
  program output.  While you are at it,
      std::cout << ´\n´ 
  (i.e. output only a char, not a string) is supposed to
  be faster.

Miscellaneous
-------------

I'm not happy to have a Borland project file with the distribution.
Usually, boost libraries don't have any build support at all,
and it seems odd to just provide a Borland project file and not,
say, a traditional Makefile.  This whole issue should be postponed
until the regression test framework is established, which should
have some kind of build support anyway.

Jens Maurer