Re: [multi] Formal Review Begins

This is my formal review of Boost.Multi.

On 3/5/26 14:26, Matt Borland via Boost wrote:
> - What is your evaluation of the design?

While the array-of-arrays view of a multidimensional array can 
(occasionally) be useful, I find the lack of direct element addressing 
using a tuple/array/vector type problematic.

The rotated/unrotated/transposed member functions are specific cases of 
the more general concept of changing the order of the dimensions.  A 
simple direct function should be added.

The use of "rotated" to switch the order of dimensions is unfortunate 
because it conflicts with the meaning of rotating an image, i.e. turning 
this:
    {{1, 2, 3},
     {4, 5, 6},
     {7, 8, 9}}
...into this:
    {{7, 4, 1},
     {8, 5, 2},
     {9, 6, 3}}
Instead, it turns it into this:
    {{1, 4, 7},
     {2, 5, 8},
     {3, 6, 9}}
...which, geometrically seen, is flipping operation over the diagonal, 
not a rotation.

"taked" is not a word in the English language.  Based on the context, I 
think the correct word is "taken".

The use of unary operators + and ~ on arrays conflicts with the 
potential elementwise use of the same operators:

   using boost::multi::elementwise;
   boost::multi_array<my_class, 2> a{2, 3};
   auto b = -a; // Creates a lazily evaluated array that applies
                // operator- to its elements.
   auto c = +a; // Creates an eager copy of the array, without applying
                // operator+ to its elements.

The name "dynamic_array" implies that it is more dynamic than plain 
"array", but the opposite is true.  This class should be renamed, 
possibly to "fixed_array" or "static_array".

The boost::multi::array::reextent function is not clearly defined. 
Given this input:
   {{1, 2},
    {3, 4}}
what happens if I call reextent({3, 1}, 0)?  Do I get this:
   {{1, 2, 0}}
or do I get this:
   {{1, 2, 3}}

I don't like the name "restriction" for a lazy array.

I don't like the name "extensions" for what I would call "extents".

The factory function "restricted" takes its argument in the opposite 
order from how the class "restriction" takes its template arguments.  Why?

The constructors of boost::multi::dynamic_array and boost::multi::array, 
as well as the function boost::multi::array::reextent, require copy 
construction.  It would be more flexible to allow emplacement from any 
set of variables that can be used to construct an element, similar to 
std::vector::emplace_back.

> - What is your evaluation of the implementation?

I only took a cursory look at it, mostly to make up for deficiencies in 
the documentation.

> - What is your evaluation of the documentation?

It's pretty bad.

On a technical level, the web page doesn't accept arrow keys or page 
up/down for scrolling, so I have to rely use my mouse for scrolling.

Also on a technical level, the right side of some of the tables is cut 
off on my web browser even though there is blank space to the right of 
the table.  I am forced to rely on horizontal scrolling to see the whole 
table, and there isn't even a horizontal scroll bar.

The tutorial recommends using "auto &&" or "auto const &" for holding 
subarrays instead of "auto".  I disagree.  A subarray is not a 
reference, no matter how much it tries to act like one, and holding it 
by reference is a good way to get a dangling reference.  I am fully 
aware that local variables of reference can extend the lifetime of a 
temporary, but I consider it messy to rely on this unless absolutely 
necessary.

The reference for functions does not include the full function 
prototype, with return type and argument types.

The boost/multi/elementwise.hpp header is not documented at all in the 
reference section.

The documentation for (I assume) 
boost::multi::subarray::const_element_ref just calls it element_ref.

boost::multi::subarray::index_range and 
boost::multi::subarray::extension_type are only described in very vague 
terms.  What are the members of these types?

The return types of boost::multi::subarray::elements and 
boost::multi::array_ref::elements should be documented.

The reference for boost::multi::dynamic_array::elements says that it is 
overridden from its base type.  This is not true, it inherits from 
boost::multi::array_ref::elements (which does override 
boost::multi::subarray::elements).

The references for dynamic_array and array have a bunch of references to 
array_ref and dynamic_array respectively for functionality that is 
ultimately inherited from subarray.  It would be better to point these 
references directly to where the functionality is defined.

boost::multi::restriction has a member function called "home" which 
"returns an indexable cursor".  I don't know what this cursor is, what 
it does, or what members it has.

boost::multi::inplace_array is mentioned once but never documented.

"operator⇐" should be "operator<=".

> - What is your evaluation of the potential usefulness
> of the library? Do you already use it in industry?

I think a good multidimensional array type is very useful, and I use my 
own multidimensional array type all the time.  The question is if 
boost::multi::array qualifies as a good multidimensional array type.

> - Did you try to use the library? With which compiler(s)? Did
> you have any problems?

No.

> - How much effort did you put into your evaluation?
> A glance? A quick reading? In-depth study?

I spent most of the day on it.

> - Are you knowledgeable about the problem domain?

I've written my own multidimensional array class and I've used it 
extensively.

> Ensure to explicitly include with your review: ACCEPT, REJECT, or CONDITIONAL ACCEPT (with acceptance conditions).

I would love to have a good multidimensional array library in Boost. 
Unfortunately, Boost.Multi is not such a library in its present state. 
At the very least, I would need the following changes before I can 
accept the library:

   - The documentation needs to be completed.
   - I want to either be able to index the array using a 
tuple/array/vector type, or a damn good rationale or why this isn't 
possible.

Unfortunately the problems with the documentation are so severe that I 
feel that another review is needed after the documentation is completed 
before the library can be accepted.  I therefore vote to REJECT this 
library at the current time in its current state.


-- 
Rainer Deyke - [email protected]

_______________________________________________
Boost mailing list -- [email protected]
To unsubscribe send an email to [email protected]
https://lists.boost.org/mailman3/lists/boost.lists.boost.org/
Archived at: https://lists.boost.org/archives/list/[email protected]/message/Y434C5IWCONZ5PIJCLWMVBBSAVFUDELW/