Re: [multi] Formal Review Begins

Klemens Morgenstern via Boost <[email protected]>
Newsgroups gmane.comp.lib.boost.devel
Message-ID <CAGT5OKPR8CS-y3vQ5iLOKXJoVjgVidatPMODjuJKPDw2F-rUCg@mail.gmail.com>
On Thu, Mar 5, 2026 at 9:34 PM Matt Borland via Boost <[email protected]>
wrote:

> Dear All,
>
> The review of Multi by Alfredo Correa begins today, March 5th and goes
> through March 15th, 2026.
>
> Multi is a modern C++ library that provides manipulation and access of
> data in multidimensional arrays for both CPU and GPU memory.
>
> Code: https://github.com/correaa/boost-multi
> Docs: https://correaa.github.io/boost-multi/multi/intro.html
>
> For reviewers, please use the master branch.
>
> Please provide feedback on the following general topics:
>
> - What is your evaluation of the design?
> - What is your evaluation of the implementation?
> - What is your evaluation of the documentation?
> - What is your evaluation of the potential usefulness
> of the library? Do you already use it in industry?
> - Did you try to use the library? With which compiler(s)? Did
> you have any problems?
> - How much effort did you put into your evaluation?
> A glance? A quick reading? In-depth study?
> - Are you knowledgeable about the problem domain?
>
>
Below is my review of boost.multi:

## Design

It also looks like a mature library written for use in a particular code
base
with different style one might expect from a boost library.
Part of that is also that authors (reviewer included) add things
to libraries that are useful for the early users
but not really worth including in a boost library.


I think the names are off, e.g. `multi::array` behaves like a `std::vector`.

I would recommend a rename them as follows:

 - `array` -> `vector`
 - `array_ref` -> `span`
 - `inplace_array` -> `array`

Furthermore, the `subarray` shouldn't exist, but just be a `span`, i.e. the
same type as `array_ref`.

I would also just remove the `dynamic_array` type. First of all: it's not
dynamic,
since it can't dynamically change its size.
Building this type with a `unique_ptr` and a `array_ref` is trivial.
I don't think there's any need.

I find the implementation a bit odd. `subarray` inherits `const_subarray`
(which is also public)
and then just casts away `const`. I don't get it, why couldn't that just
behave like `span`
with `const T` and `T`?

I'd imagine a `array_ref<T, 3>` just needs to hold a `array<size_t, 2>
strides_` as members.

What is the point of `extensions()` [probably should be `indeces`] ?
Is this just so I can use a range based for loop instead of the old-school
one?


I think it's unfortunate that `array_ref` only accepts a `pointer` and a
set of `extends`.
I would really like to have a "contiguous container + strides" constructor:

  std::vector<int> vec {1,2,3,7,8,9,4,5,6};
  boost::multi::array_ref<int, 2> arr({3}, vec); // deduce the number of
rows


The types have special handling of an array size of `1`.
I don't know why this is needed at all, what's the use-case?

Why is `data()` restricted to the single dimension array?

I don't like the algorithms (`diagonal, `slice`, etc.) being members.
I prefer them as free functions in old-school fashion like so:

  auto slice(array_ref<T, N> &, size_t first, size_t last)

Or

  array_ref<T, N-1> diagonal(array_ref<T, N>);

For those who fance daisy-chains a `std::ranges` compatible API could be
provided.


Is the term `restriction` a general one or specific to this library?
It looks like `generator` or `expressions` might be better terms,
but maybe it's domain specific terminology I am unaware of.

## Implementation

I find the code hard to read because it's full of indirections
and base classes that seem to have a single use.
I think all of this could be simplified, but since it's not mine to
maintain, that doesn't matter for this review.

There are some oddities in the code, liks this:

constexpr auto operator=(array_ref&& other) &&
noexcept(std::is_nothrow_copy_assignable_v<T>)
-> array_ref& {
if(this == std::addressof(other)) {
return *this;
}  // lints(cert-oop54-cpp)
operator=(std::as_const(other));
return *this;
}

If we're doing this check, why is it only done in the non-const version?



I also looked at a few test files and they looked sufficient.

What's `multi/algorithm/redux.hpp` for?
`multi/io.hpp` should #include `iosfwd` not `iostream`.


## Documentation

The docs are alright, except for the reference section.
E.g. we only get the function names, but not the argument list or
return_type.


## Usefulness

I like this library and I think I'll use it after the review independent of
outcome.
I currently hand-coding this stuff at the moment.

## Usage

I used gcc 15.2 and all the tests passed.

I tried a few minor things and it worked as expected.

## Effort

I spend about 3 hours reading the docs, looking at the code
and building some toy applications.

## Knowledge

I am doing rather simple things with 2D & 3D arrays,
so I should not be considered an expert here.

## Conclusion

I am a bit conflicted, I see three major issues:

1. Many names are off, in a way that goes beyond bikeshedding.
We have `std::array<int, 3>` and `multi::array<int, 3>`,
and both do fundamentally different things.
Restrictions don't restrict and extensions to extend.

2. Some conventions don't seem to fit boost.

3. The reference section is not helpful.


The library is definitely useful, but too idiosyncratic for boost.

Therefor, I recommend to CONDITIONAL ACCEPT the library with the following
conditions:

1. Rename the types so they are intuitive to C++ developers, for example as
described above.
2. Remove or generalize (make available to all N) functions currently only
available for N =1 (e.g. data())
3. Make array_ref and subarray the same type.
4. Provide algorithms as free functions.
5. Remove the `extensions()` function if it is the same as a C for-loop.
6. Remove `dynamic_array` or give it a better name like `unique_array`.

I am not making the reference a part of the conditions, because "make it
better" is not a condition
that can be fulfilled without another review.
_______________________________________________
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/ZEDHGGWHF6TXBHJ6GXHNXGU2CYI2JMTI/
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.