Re: [boost] array formal review

From: Jens Maurer (Jens.Maurer_at_[hidden])
Date: 2000-06-26 16:33:30

Ok, here are my review comments before the time runs out.

In general, I think the class is well thought-out and simple.
The examples are ok.

I vote for inclusion into boost.

I'm not sure about the name, but for me "c_array" rings more
bells than simply "array".  "block" is definitely out of the
game IMHO. Oh, and "carray" is too munged for my feeling.
But anyway, I don't really care much.

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

array.html:
  Can we stick more closely to the boost look & feel for that page
  and have (at least) the boost GIF on top? Maybe even the navigation
  bar?

  "many feedback": Is it really countable? I thought it was
  "much feedback".

  "Class array fulfills most but not all of the requirements of
  reversible containers (see Section 23.1, [lib.container.requirements]
  of the C++ Standard). The class is not an STL container because ..."
  This tells you that it isn't quite a reversible container and then
  it explains why it isn't a container. Are there any other things
  required for a reversible container (as opposed to a simple container)
  which are missing from the library? [No, there aren't: please say so.]

  Instead of "Home Page", I would appreciate the author's name
  clearly visible on the web page.

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

array.hpp:
  Comment after #include´s:  // BUG-FIX for compilers that doesn't
support
  should say "... don't support" (plural!)

  The typedefs in namespace std are non-conforming (because they add to
  namespace std in an invalid manner) and they are not guarded by
  #ifdef BROKEN_COMPILER (whence this would be tolerable, see
config.hpp).
  Additionally, multiple such typedefs (for example, if another
unrelated
  heaeder file had the same idea) violate the one definition rule.
  Summary:  #include <boost/config.hpp> and forget about the issue.

  The rangecheck() member function should be declared "const", otherwise
  the call from the const at() member function looks invalid.

  As mentioned elsewhere, make size(), empty() and max_size() static.

  Do we want the "using std::swap;" trick so that Koenig lookup finds
  more global element swap() functions?

  "// comparisions": My dictionary says the second "i" is superfluous.

array1.cpp:
  The example should reference "boost/array.hpp" and not simply
  "array.hpp".  I don't care for <...> or "...".

  We should not use std::endl too often, because it calls flush(),
  which is usually a performance bottleneck.  Nearly all output
  statements can be coalesced to one big expression.  After all,
  it's an example program, so it should show good manners :-)

array2.cpp:
  I don't like "using namespace", and particularly not in
  example programs.  I favour putting "std::" where it is
  required.

  PRINT_ELEMENTS() uses all caps, which should be reserved for
  preprocessor symbols.

array3.cpp:
  Has another print_elements() implementation.

print.hpp:
  Incorporate array3.cpp´s print_elements() into array2.cpp
  and do away with print.hpp, since it's used only in array2.cpp
  anyway.

Enough for now.

Jens Maurer