Date: Thu, 12 Mar 2026 17:02:50 +0100 From: Ruben Perez <rubenperez038@gmail.com> Subject: [boost] Re: [multi] Formal Review Begins To: "Boost developers' mailing list" <boost@lists.boost.org> Message-ID: <CACR-md+BUG= 4WSEzNJ5ZYJv6jetTvkhKVGQLmNC0ZzK3AMVK+Q@mail.gmail.com> Content-Type: text/plain; charset="UTF-8"
On Thu, 5 Mar 2026 at 14:37, Matt Borland via Boost <boost@lists.boost.org> wrote:
Dear All,
The review of Multi by Alfredo Correa begins today, March 5th and goes
through March 15th, 2026.
Hi all, Let me start with a personal apology to Rubén Perez; I completely missed his review from March 12, and it came across as me ignoring it, although it was really constructive in retrospect. I answered most reviews as GitHub issues, but since I didn't see his issue there, it slipped from me. Like the original reviews and his re-review from a few days ago (7 August), I answered in a GitHub issue. So when Ruben said in his August 7 review that "I feel the library is at a similar point as last time I looked at it", he is completely justified, because I accidentally didn't address any of his points. But given the special situation, I feel obligated to give a detailed response to this first review in March.
This is my review of the proposed Boost.Multi. Thanks Alfredo for submitting the library and Matt for managing the review.
Thank you, Ruben, for reviewing my library, and apologies again for not addressing your original review from March. (That is why it feels that the library didn't change at all from your point of view).
- 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.
Thanks
- 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.
This is answered in the new review in the Github issues (August). Yes, interaction with std::mdspan improved. Arrays and subarrays are implicitly convertible to std::mdspan<T, std::dextents<std::size_t, D>, std::layout_stride> following the current design of the relationship between std::vector and std::span.
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.
API reference section is now generated automatically with MrDocs; conceptual things that are not well captured by automatic tools have their own section: https://correaa.gitlab.io/boost-multi/multi/reference.html
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.
The typedef multi::size_t has been removed to avoid confusion. The index type is a class typedef, e.g. `multi::array<T, D>::index`.
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.
Index signedness and negative indexing are separate decisions. The index signedness is a principled decision independent of other features.
* 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.
Things related to ptrdiff are derived from the allocator type. If I hard-code std::ptrdiff_t, the connection with the allocator will then be lost. In the same way I have array::index (index type), there is array::difference_type.
* 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.
I think this is a problem with fmt. Although it works for me, at least for basic uses of fmt. (this is addressed in the new GitHub August issue) * 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.
This is the issue: you allocate 12 elements for a 3x4 array; therefore, there are 13 addressable locations (marked with `o` below). oooo oooo oooo oabc The end() iterator of the first column is ok, it sits in an `o` location. The end() iterator of the second column "sits" in `a` which is not a valid address because it beyond the 12 element allocation (that has only 13 valid addresses). I rewrote the section about UB before the last (August) review. * 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").
I understand; the logic is the opposite here and analogous to initialization of single elements The idea is that you can be explicit about non-initializing elements. Always remember that neither use leads to uninitialized elements for types that cannot afford it. More about this is explained in the GitHub issue (August) * 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.
This is a consideration for when the library is bumped to C++20; at the moment, I have annotated all the locations that will be replaced with requires clauses. The guarded alternative you propose adds too much noise to the code and forces the maintainance of two parallel concept systems. * 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().
Actually, after the fact, I was informed that NumPy uses the same convention; unary + materializes an instance of the array. This is the dilemma with .copy(): it gives the impression that the library needs explicit .copy() to copy an array, implying that it needs an explicit mechanism to work around some sort of reference counting, which the library doesn't do anyway. All this is needed because of a limitation of auto rather than because of a technical need of the library. BTW, the function is called .decay() in the library. Giving two options, `+` or `.decay()` if you want to be more explicit.
* 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.
Actually, making the reference types copyable leads to surprises in my opinion. This is addressed in the GitHub issue, and I added a new section called "Addressing subarrays".
Maybe this is my lack of understanding, but subarray and array_ref seem similar enough to be implemented using the same class.
array_ref has a simpler interface because it deals with continuous memory. You are right that they can be implemented mostly in the same way, and they are.
- What is your evaluation of the implementation?
It's difficult to follow, and it has a non-trivial amount of technical debt.
Yes, it is a complicated library to implement. Most of the points below are addressed in the Github issue (August) 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?
The rationale is that std::tuple doesn't work on the GPU (__host__ __device__ etc), so I have to implement it from scratch.
- 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.
The sections have been renamed, and the public interface is documented through MrDocs.
I'd also try to introduce concepts incrementally. For instance, slicing should probably come before assignment, since you use slicing to discuss assignment.
I feel that I can't delay the assignment section too much, so what I do to address this is to just explain what the syntax uses does and refer the reader to a later section for a full explanation.
Some minor points:
* I'd advise using snake_case for variables in the examples.
That is what I am doing in general; maybe I changed this already. I feel that saying things like A2D or A2 is ok; A_2D would be too verbose. * https://correaa.github.io/boost-multi/multi/tutorial.html#tutorial_init
Prefer std::unique_ptr to raw new and delete in the examples.
I think it will distract from the main point: First, it will not make it safer because then the scope of the array references will not be delimited to a clear delete, Second, typical uses of array_ref involve legacy code that doesn't manage memory automatically Third, I will have to use .get() to get the raw pointer.
* Some sections refer to the "std::mdspan proposal". It's already in the standard, so this should be updated.
I can't find myself referring to "std::mdspan" as a proposal; maybe I changed this already.
* The BIP example in the interoperability section seems to contain unbalanced scopes (unmatched '{}' characters).
The scopes are balanced; there was a stray commented "}" that could be causing confusion. *
https://correaa.github.io/boost-multi/multi/interop.html#interop_substitutab... " In a departure from standard containers, elements are left initialized..." should be uninitialized.
fixed *
https://correaa.github.io/boost-multi/multi/tutorial.html#tutorial_slices_an... "Other notations are available, for example this is equivalent to A(multi::" contains a rendering error.
fixed *
https://correaa.github.io/boost-multi/multi/interop.html#interop_serializati... "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".
Fixed, and similar problems like "Here it is" * "Gettings started" should be "Getting started" in the nav bar.
fixed * reinterpret_cast_array is mentioned in the docs but it does not exist. It is now documented as "yields a view of the subarray where elements are reinterpreted as a different type (elements must have compatible size)"
- 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.
I would like to see the compilation error you get, here or in godbolt.
- 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.
Thank you; that is a lot of work, and I take your review seriously, and I apologize for missing it initially.
- 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.
Thank you again
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.
Thank you, Rubén; your insight will always be welcome, whatever the outcome. Please let me know if you have any questions. Alfredo