Review: Multi-Array Library

From: Douglas Gregor (gregod_at_[hidden])
Date: 2002-01-03 00:05:49

My review of the Multi-Array library follows. I read through the 
documentation, tried a few examples, but was unable to give the source code a 
careful reading due to time limitations.

Overall, I think that the Multi-Array library should be accepted into Boost. 
More detailed comments follow, but my vote for acceptance is not conditional 
on anything stated below. I believe that the library is in excellent shape 
as-is, and I found the interface to be very intuitive.

General documentation comments:
  - I dislike the "almost" conformance to RandomAccessIterator; not because 
there is an inherent problem with not having a true reference, but because 
"almost" modeling to a concept doesn't really give us any information about 
the concepts that are modeled. I'd much prefer to see a separate concept or 
set of concepts enumerated that it does fit, and within those concepts the 
differences from RandomAccessIterator can be explained. Jeremy's new iterator 
categories would be a reasonable set of concepts to use: then the 
multi_array::iterator types would model RandomAccessTraversalIterator, 
ReadableIterator, and (for non-const) WritableIterator.

  - At several points, const_iterator is said to model OutputIterator. This 
should be InputIterator.

Main documentation (index.html):
  - I dislike the phrase "or fatal as under MSVC." Compiler bugs may be 
obnoxious, but we shouldn't be taking shots at vendors within documentation.

MutableMultiArray Concept documentation:
  - index_range and extent_range have the exact same documentation string, 
which can lead to confusion (it did for me).
  
  - The "Post-condition" column need not exist unless it is going to be used.

  - The origin() function does return the address of "a[0][0]...[0]", unless 
all of the indices are zero. It returns the address of "a(a.index_bases())" 
(?).

  - I think that the MutableMultiArray and ConstMultiArray concept documents 
should be merged into a single MultiArray concept document, with comments 
describing the (relatively few) differences. This is just a matter of 
preference, of course.

multi_array class documentation:
  - Overall: I would like to see Standardese documentation (i.e., 
Requires/Effects/Postconditions/Returns/Throws/Complexity/Rationale)
  - What happens when the dimensions don't agree when the assignment operator 
is called? Undefined behavior? Exception thrown?
  - The assign(values) form (that assigns to the multi-array from elements in 
the sequence [values, values + num_elements())) strikes me as being _very_ 
dangerous, because it would be very easy to overrun your input buffer. I 
would (strongly) suggest removing this form.
  - Does assign(first, last) require that std::distance(first, last) equal 
num_elements()? I believe that it should, because a partial assignment is not 
an assignment, and generally it signifies a user error if one tries to assign 
to a multiarray a different number of elements than the multiarray can 
contain.
  - The operator()(indices) and data() member function descriptions make that 
same mistake as in the origin() function, mentioned above.

Code comments: 
  - Why is it that the concept checks are commented out of the source (i.e., 
in multi_array.hpp)?
  - There were some problems compiling with Comeau's strict mode, but I will 
contain the author directly with those fixes. They aren't important enough to 
warrant mailing list traffic.

        Doug