Re: [multi] Formal Review Begins

On Thu, 5 Mar 2026 at 14:37, 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.



Hi all,

This is my review of the proposed Boost.Multi. Thanks Alfredo for
submitting the library and Matt for managing the review.

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

I think that it has the potential to be very useful.
I don't use it today, but having it would have been beneficial
for one of my former clients. IMO a library like this should
have a place in Boost.

> - What is your evaluation of the design?

I think the library got its core decisions right. Iterating,
indexing and slicing are elegant. Lazy arrays are great.

Interaction with std::mdspan could be better. At the moment,
we have a godbolt link with some non-trivial code. mdspan
is already standardized (C++23) and available on some standard libraries.
Functions for easy interoperability should be provided.
C++26 linalg [1] is based on std::mdspan, so providing this will be important
in the future.

As other reviews have mentioned, the reference section is incomplete,
and there are a lot of types and functions in the public boost::multi
namespace that are not documented. This creates a gap on what I can evaluate
in this review. I don't know if many of these functions (e.g. data(),
data_elements())
are supposed to be used by the end user or not.

Aside from this, there are a number of surprising decisions that
have the potential for creating trouble:

* Indexing uses signed rather than unsigned integers. From a conversation
  with the author in Slack, signed-ness was chosen to allow negative
  indices (like Python does). The problem is that the actual type is buried
  within multiple layers of type aliases. One of these aliases,
  multi::size_t, is actually signed, making things even more confusing.
  I'd advise to:
    * Try to implement the negative indexing feature before anything else.
      It doesn't sound like a trivial thing, and should probably be done
      before committing to API stability.
    * Remove as many type aliases as possible. Use std::ptrdiff_t everywhere,
      which immediately tells the user the type that they're using.
      Especially multi::size_t.
* Arrays overload the "address of" (operator&). I don't think this is good
  practice at all. It's also not mentioned in the docs. With my system
  fmtlib (v9.1), printing subarrays fails to compile because of this.
  While it's true that fmtlib should be using std::addressof instead
of raw operator&,
  we could avoid the problem altogether.
* Functions like strided() can lead to non-obvious undefined behavior.
  I don't think this should be the case. If the performance gain
(after measuring)
  is significant, an strided_unsafe() can be added.
  Docs mention "The third case is that of iterators of transposed
array." regarding
  UB, which I don't really understand.
* Initializing and resizing arrays leads to uninitialized elements by default.
  I think this default is confusing, especially because there is a
multi::uninitialized_elements
  tag. I'd expect that elements are initialized (safe default) unless
I explicitly use
  multi::uninitialized_elements ("I know what I'm doing").
* Many library functions and classes have lots of template parameters.
  I'd advise using C++20 concepts (properly guarded in C++17 builds) to
  improve error messages and document the requirements for each type.
* I personally don't like unary operator+ for making copies. It's not
communicating
  intent. I'd go for what numpy.ndarray does and call it array::copy().
* Subarrays and array references are not copyable. This is unusual,
since all reference
  types I know of (span, mdspan, string_view, url_view,
mysql::rows_view) are copyable.
  It looks like the library tries to make the type appear like a reference, but
  I don't think that's wise - exotic references are not language references,
  no matter how hard you try.
  I would make subarrays copyable to avoid surprises.

Maybe this is my lack of understanding, but subarray and array_ref seem
similar enough to be implemented using the same class.

- What is your evaluation of the implementation?

It's difficult to follow, and it has a non-trivial amount of technical debt.
Some points to note:

* include/ contains many files that are not headers. It contains docs,
  build scripts, CMakeLists.txt and empty files. include/ shouldn't
  contain any of this because it's installed with the library to the consumer.
  Some of the headers under "adaptors/" seem to be public API, others
  seem to be examples or tests.
* There are many files containing commented leftover code and
  #if 0 preprocessor blocks.
* There is an unscoped, undocumented NOEXCEPT_ASSIGNMENT macro in array.hpp.
  This needs either to be renamed to BOOST_MULTI_NOEXCEPT_ASSIGNMENT
and documented,
  or (better) removed.
* Same with MULTI_USE_HIP (although I understand that this one may be
unavoidable).
* The min and max functions need to be guarded against macro substitutions.
* None of the examples seem to be built by CI, and some are not in the
CMakeLists.txt either.
  They need cleanup and comments to explain what's going on.
* A considerable amount of the test suite is missing from the test Jamfile.
  This is problematic because your main source of compiler coverage happens
  by running b2, not CMake.
* "examples/" should be renamed to "example/".
* There seems to be an operator referencing dynamic_array_cast,
  but I can't find this function defined anywhere. It seems to be mentioned
  by the docs, but the function isn't there.
* What's the rationale behind implementing a custom tuple type?

- What is your evaluation of the documentation?

As other reviewers mentioned, the reference needs to be reworked.
I won't insist here, since it's already been mentioned.

Public adaptor code needs to appear in the reference, too.
Same for config macros that might be defined by the user.

The discussion is useful and explains the rationales well.
I'd change the "Advanced usage" section title, since constructing
an array is not advanced usage.

I'd also try to introduce concepts incrementally.
For instance, slicing should probably come before assignment,
since you use slicing to discuss assignment.


Some minor points:

* I'd advise using snake_case for variables in the examples.
* https://correaa.github.io/boost-multi/multi/tutorial.html#tutorial_init
  Prefer std::unique_ptr to raw new and delete in the examples.
* Some sections refer to the "std::mdspan proposal". It's already in the
  standard, so this should be updated.
* The BIP example in the interoperability section seems to contain
  unbalanced scopes (unmatched '{}' characters).
* https://correaa.github.io/boost-multi/multi/interop.html#interop_substitutability_with_standard_vector_and_span
  " In a departure from standard containers, elements are left
initialized..." should be uninitialized.
* https://correaa.github.io/boost-multi/multi/tutorial.html#tutorial_slices_and_strides
  "Other notations are available, for example this is equivalent to
A(multi::" contains a rendering error.
* https://correaa.github.io/boost-multi/multi/interop.html#interop_serialization
  "Large datasets tend to be serialized slowly for archives with heavy
formatting. Here it is a comparison of speeds when serializing and
deserializing a 134 MB 4-dimensional array of with random double
elements."
  Should be "Here is..." and "4-dimensional array of random double elements".
* "Gettings started" should be "Getting started" in the nav bar.
* reinterpret_cast_array is mentioned in the docs but it does not exist.

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

I've run some of the examples in the docs in my Ubuntu 24.04
machine with clang-22, C++23, libc++ and CMake in Debug mode.
No problems aside from the fmtlib problem with subarrays already discussed.

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

I've read the documentation, cloned the library, scanned the source code for
things that called my attention and built some toy examples. I haven't
delved into the GPU adaptors because it's outside my knowledge.
I've spent around 8h with the review.

- Are you knowledgeable about the problem domain?

Not an expert. I've done some very basic scientific programming in C++.
My main expertise is networking.

> Affiliation disclosure

I'm currently affiliated with the C++ Alliance.

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

My current recommendation is to REJECT the library at this point.
This is not a hard rejection. I'd like to see this library being reviewed again
in 2-3 months, when the points raised in the review are fixed. Similar to
what we did with Boost.Decimal. I was close to recommending
conditional acceptance though, so please keep up the good work.

Regards,
Rubén.


[1] https://en.cppreference.com/w/cpp/numeric/linalg.html
_______________________________________________
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/Q4GLGQVJ26LVQE5INMF73PKBSJ67FUCH/