Re: [boost] Multi Array Review

From: Jens Maurer (Jens.Maurer_at_[hidden])
Date: 2002-01-03 17:11:10

John Maddock wrote:
> 
> The first in a series of reviews begins today, with a review of Ronald
> Garcia's multi_array submission.

Here are my review comments.

In general, I believe the library should be accepted into boost.
However, there could be some overlap with the mtl library.
I'd be happy if someone familiar with both could state that
the interface of Boost.MultiArray allows iterators to be defined
that are efficiently usable with mtl.

Details below.  (Some of the details may be repetitions from other
people; I haven't had the time to remove the duplicates.)

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

index.html:
 - introductory paragraph:  It sounds confusing to me.  First, you say
it's "equivalent" to a "vector of vectors", then you say you can't
resize a multi_array.  I believe resizing is a major feature of
std::vector.  A more careful wording might be in order.

 - The links to array_traits and boost::array are empty (and thus broken).
More links are broken elsewhere.

 - Move the concepts section way up to just after the rationale.  It's
more standard-like and (IMO) easier to grasp a library top-down.

 - I support merging the two concepts documents.

 - I would appreciate if the example code (particular in the
docs) would refrain from using the token 
"array", since it
might be confused with Boost.Array's template class.
"marray" seems ok.

 - In the example code, the loop exit conditions should read
   "n < 3" instead of "n != 3".  The former is, IMO, more
customary and doesn't break if "n" happens to be 10 at the
beginning of the loop.

 - The longish "associated types" listing is redundant
with the concepts documentation and should be removed from
index.html.

 - "Each array can be assigned to from any of the others," is
certainly incorrect for const_multi_array_ref.

 - "Array View and Subarray Type Generators":  The first two
sentences are misleading.  I know of no situation in ISO C++ where
member templates cannot be used.  If you want to say that
some broken compilers have a problem, then say so straightforwardly,
e.g. "The additional template keyword ... might be considered
obfuscating.  Also, some compilers cannot handle member templates."

 - "Specifying Array Dimensions":  It appears to be dangerous
to blindly retrieve N elements from the collection.  I'd rather
have it retrieve from begin() to end() with the requirement
that size() == N.

 - I'm in favor of chaning the class documentation to the
usual layout of the C++ standard.  It's more concise, more
precise, simply better.

MutableMultiArray.html:
 - "It attempts to refine RandomAccessContainer" is much too vague
to be useful.  Just start out saying what it *does* refine
(you don't say that at all), and have a list of differences
(the "not quite") with RandomAccessContainer at the end.
Also, the "motivation" is mostly redundant with index.html and
could be either moved there or removed altogether.

 - Please reorder the various tables ("associated types" etc.) so that
the number of forward references is minimized (".. the same as element").

 - NumDims is never introduced, just used.  Please introduce it before
the tables start, e.g. "In the following tables, X is a type that
models MutableMultiArray; NumDims is an integral constant expression
with value > 0, ...".  Similar for the "expression" table: Lots of
variables used without defining them.

 - size_type and difference_type need more explanation
(or refer to the std lib concept that you refine).

 - As said elsewhere, the iterators that you provide need
better definition.  "almost" is a no-no in reference documention.

 - I don't like the index and index_gen names.  They don't convey
the information that they are types, not values.  Usually, "index"
would be a variable name in my mindset.   index_gen could have
"range" in it (index_range_type?)

 - I'd merge the "expressions valid" and "expression semantics"
tables and omit the post-condition column if unused.
The "Name" column is largely redundant.

 - "if a is mutable": Is this standard-speak?  Shouldn't it be
"const" and "non-const"?

 - The size, number of dimensions, and number of elements
appear to be compile-time quantities.  They should be available
at compile time as well, not just with a function call.

 - Shapes, Strides, Index Bases returns const something*, which
appears fishy to me.  Wouldn't an iterator-based iterface more
in the spirit of C++?  begin_shapes(), end_shapes() etc.

 - "Collection" is not a concept defined in the C++ standard.
So you should fix the link, and make sure it points to somewhere
within boost.

 - What's "indices"?  It appears to be a library-provided
variable.  So you should say "boost::indices" so that people
know where to find it.

 - rend(): No iterator may point to before the first element.
You may want to replace the verbiage with the C++ expression
whose value is returned.  Similar for rbegin().

 - origin(): Why does it return a pointer, and not a reference?

 - The complexity guarantees should be integrated into the
tables.  size() being at most linear with MultiArray's size
sounds frightening, and vague (number of elements?).

 - The invariants could say more about the relationship
of size(), strides(), shape(), index_bases() etc.  Also,
how the a[idx] indexing relates to the a(index_list) indexing
method.

Code
----
 - Needs attention to the fact that classes having dependent base
classes don't have the members of the base class visible for
unqualified lookup.
 - The "global" status/regression.cfg will likely only contain one
test for this lib.  So you may want to define a all_test.cpp (or
somesuch) that #include's all supposedly working test source files.
(The separate regression.cfg with the detailed tests is nice and
should be kept as well.)
 - Please augment the fail_xxx.cpp tests with a comment that
says where and why it should fail.  Oh, and if it fails
early because assignment is not allowed for a const object,
there's no need to have all the "verify" stuff in there.
That's all for now.
Jens Maurer