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