multi-array review

From: John Maddock (John_Maddock_at_[hidden])
Date: 2002-01-10 07:16:29

The following comments are my own personal views and should not be confused
with my role as review manager.

First the headline: I think that multi_array should be accepted into boost.
 The following comments are mainly based on an examination of the docs, as
multi_array doesn't build with my favoured compiler (Borland C++), BTW I
can fix most of the Borland compiler issues but I just can't get the
num_dimensions method to build - any Borland experts see the problem?

In the introduction you say:

"iterator 

A type that dereferences to return objects of type reference. If NumDims ==
1, then this is a type that models Random Access Iterator. Otherwise it
models both Input Iterator and Output Iterator (it almost models Random
Access Iterator, but reference is not a true reference type). "

What are the differences between the iterator type and a random access
iterator? Why can't it be a true random access iterator? I assume that it's
because the iterator returns a temporary proxy object, rather than a
reference, if so say so.

The following doesn't immediately make sense:

"The second method involves passing the constructor an extent_gen object,
specifying the matrix dimensions. By default, the library constructs a
global extent_gen object boost::extents. In case of concern about memory
used by these objects, defining BOOST_MULTI_ARRAY_NO_GENERATORS before
including the library header inhibits its construction. "

I assume that boost::extents is a global that can be used to avoid the
construction of a temporary extent_gen object? If so say so.  This looks to
me to be non-thread safe as well (so say so), maybe this isn't such a good
idea anyway?  Does constructing a temporary extent_gen really add so much
overhead, or is it just syntactically poor?  If the latter then maybe there
is a better way somehow?

Under element access you have two methods:

"The second method involves passing a Collection of indices to operator().
N indices will be retrieved from the Collection for the N dimensions of the
container. "

Is this more efficient that the first, or just a syntactic alternative?

You say that: "Each array class provides constructors that accept a storage
ordering parameter. "

Is this the best design choice - could element access be made more
efficient if the storage order were a template parameter?  Are there any
situations where you would need to access the same array but with different
storage orders, I can't think of any, but maybe you can?  This is a
question not a criticism BTW :-)

The following is a minor point, but I would have preferred the array
concepts to use the same table format as the standard, using three
sections: expression, return type, and description.

In the intro the link to array traits in the sentence "With the help of
array_traits, you can even extract " is broken, in fact there appears to be
no docs for this class? If so that really needs fixing.

In multi_array.html the heading:

"template <
   typename Element,
   std::size_t NumDims,,
   typename Allocator = std::allocator<Element>
  >
multi_array"

Should be a valid forward declaration, it looks odd as is.

The same in multi_array_ref.html and const_multi_array_ref.html

I'd also like to be able to get quickly to the reference pages (maybe from
the table of contents), as it is you have to read down and then follow a
link in the text.

I like test_cases.html, maybe a similar page for the examples would be
useful.

Performance
~~~~~~~~~

A lot has been said about this elsewhere, I would only say that I would
like to see some performance comparisons in the test programs, and some
indication in the docs on how to get the most out of the library - how do
the different access methods compare - using operator[], compared to an
index collection, compared to enumerating iterators, compared to C-arrary
access etc.  Does access time increase with the number of dimensions etc? 
For the future, I would like to see some work on improving the performance
of operator[], but that's an extra feature request not a review requirement
- we don't even know if it's possible at present.

- John Maddock
http://ourworld.compuserve.com/homepages/john_maddock/