[Review 2] Ruben Review Tasks

[mborland]

This is my re-review of Boost.Multi. I'll be checking
the points that I raised during my previous review [1]
and a couple more new findings.

• The discussion has improved since last review.
  The page on concepts [2] is worth reading.
  Although it is called "reference", but it is not.
• The reference section [3] needs a lot more work.
  The front page needs a table organizing the entities
  (see how Boost.Url does it, for example [4]).
  There must be no entries with blank documentation.
  All adapters are also missing there.
• I like seeing std::mdspan compatibility through multi::array::to_mdspan(),
  but the function is absent from the documentation. There is an example in
  Godbolt about it that doesn't compile.
• Examples have no comments yet, and are still under examples/ (rather
  than example/),
  and aren't built by CI.
• The operator& overloads are still there, and subarrays still don't work
  with fmtlib (the example [5] doesn't built in my machine).
• strided() with a stride not divisible by the corresponding dimension
  causes an infinite loop in my machine. I was trying to see if Asan complained
  about the invalid pointer formation issue [6]. It doesn't, but the code
  misbehaves. The following causes an infinite loop:

#include <boost/multi/array.hpp>

namespace multi = boost::multi;

int main() {
multi::array<double, 2> arr = {
{1.0, 2.0, 3.0},
{3.0, 4.0, 5.0},
{6.0, 7.0, 8.0},
};

for(auto&& elm : arr.strided(2)) {
std::cout << elm.size() << std::endl;
}
}

• strided() with negative indices and dimensionality greater than 1
  causes an infinite loop, too:

#include <boost/multi/array.hpp>

#include <iostream>

namespace multi = boost::multi;

int main() {
multi::array<double, 2> arr = {
{1.0, 2.0, 3.0,  4.0},
{3.0, 4.0, 5.0,  8.0},
{6.0, 7.0, 8.0, 10.0},
};

for(auto&& subarr : arr.strided(-1))
std::cout << subarr.size() << std::endl;
}

• Uninitialized elements still happen by default.
  The library has gained a trait to treat std::complex
  as trivial:

template<> inline constexpr
bool multi::force_element_trivial_default_construction<std::complex<double>>
= true;  // should be defined as early as possible

This is only well-defined in C++20 (and UB in C++17). It is also
prone to ODR violations, especially since the specializations are controlled
by a macro definition.

• The include/ directory still has lots of files that don't belong there.
• There are still many #if 0 preprocessor blocks.
• The NOEXCEPT_ASSIGNMENT macro is still there.
• The min and max functions are still unguarded.
• Many tests are still missing from Jamfile.
• The dynamic_array_cast leftover is still there.

I feel the library is at a similar point as last time I looked at it, so
I keep my original vote: REJECT with the encouragement of a re-review,
once the feedback is incorporated into it.

Regards,
Ruben.

[1] https://lists.boost.org/archives/list/boost@lists.boost.org/message/Q4GLGQVJ26LVQE5INMF73PKBSJ67FUCH/
[2] https://correaa.github.io/boost-multi/multi/reference.html
[3] https://correaa.github.io/boost-multi/multi/reference/index.html
[4] https://www.boost.org/doc/libs/latest/doc/antora/url/reference.html
[5] https://correaa.github.io/boost-multi/multi/interop.html#interop_fmt
[6] https://correaa.github.io/boost-multi/multi/technical.html#technical_iteration

[correaa]

• The discussion has improved since last review.
  The page on concepts [2] is worth reading.
  Although it is called "reference", but it is not.

The section has been renamed "Reference of Types and Concepts" with a note at the type that reads This section is an conceptual overview of the system of types, if you want to read the API interface as coded, please visit the xref:reference:index.adoc[API documentation page].

[correaa]

• The reference section [3] needs a lot more work.
  The front page needs a table organizing the entities
  (see how Boost.Url does it, for example [4]).
  There must be no entries with blank documentation.

This is automatically generated documentation, besides the blank entries, which are incomplete, and I agree, I don't see a fundamental difference with Boost.URL: all the top level types are listed in the front page. The difference I see with Boost.URL is that it list things a two column format.

To be fair, the two column format actually hides empty documentation entries in Boost.URL: for example, https://www.boost.org/doc/libs/latest/doc/antora/url/reference/boost/urls/no_value.html .

I will leave this box unchecked while I complete the empty entries and waiting for feedback.

[correaa]

• I like seeing std::mdspan compatibility through multi::array::to_mdspan(),
  but the function is absent from the documentation. There is an example in
  Godbolt about it that doesn't compile.

There are several issues here:

a) Thank you, the Godbolt link (for reference here: https://godbolt.org/z/aocn73bzf)
doesn't work because std::mdspan is a moving target that is why "experimental" and "preliminary" wording was used. At the time of writing the only available version online was the version from Kokkos which interferes with the now available std/mdspan include.

b) it was no documented because the documentation was generated assuming C++17, which is also for engineering reason the lowest supported version of the library. I am now generating the documentation under C++23 so these entries will show up. Of course these will lead C++17 users to complain that some documentation entries are not available. The compromise I achieved is to say that such and such is only available in C++20 or C++23.

In other words, I am trying to support conversion to mdspan by popular demand although it is a very new feature and it is outside the supported version of the library.

c) The other reason is that to_mdspan actually leaked from the implementation because I am adopting the same convention as std::vector which has an implicit conversion of std::span but not a named function. (I guess the reviewer found to_mspan by inspecting the implementation since it is not really documented.

[correaa]

• Examples have no comments yet, and are still under examples/ (rather
  than example/),
  and aren't built by CI.

Examples have not been modified because I completely missed your review from March https://lists.boost.org/archives/list/boost@lists.boost.org/message/Q4GLGQVJ26LVQE5INMF73PKBSJ67FUCH/

I deleted the examples directory; it added too many unreasonable dependencies.

[correaa]

• The operator& overloads are still there, and subarrays still don't work
  with fmtlib (the example [5] doesn't built in my machine).

Operator& is fundamental to the design in the sense that it is necessary to distinguish value from pointer-level indirection.
If it is of any consolation, the main use of operator& is on rvalue references, which do not exist in the language, so they don't interfere.

The use is illustrated in the following diagram:

and in the example: https://godbolt.org/z/K15zeoaPj

This resolves long-standing arguments about std::span (and std::mdspan) semantics that are still heavily debated (https://open-std.org/JTC1/SC22/WG21/docs/papers/2018/p1085r0.md and subsequent), so there is no community consensus on a solution, so I choose/propose my own.

My opinion is that these are Byzantine arguments without a principled solution; so I propose a resolution at the cost of a non-common overload that mostly doesn't interfere with the language built-in operator& (because it works on r-values).

The problem with fmt seems to deserve its own issue, especially if the library decides to erroneously use operator& in place of std::addressof.
In any case, I was able to run the example with fmt 12 and with fmt trunk.

https://godbolt.org/z/7dcKxa8of

fmt is a moving target as well, so if we decide that it is more confusing than illuminating, I can remove the section.

[mborland]

As you go through these I see that you're copying down individual tasks for individual comments which is good. When you check them off can you also check off the corresponding one in my initial comment so that they're all aggregated rather than having to search all the comments for which are complete or not? Thanks

[correaa]

• strided() with negative indices and dimensionality greater than 1
  causes an infinite loop, too:

After some deliberation, I decided not to support non-divisible strides.
Defining the result creates too many edge cases related to negative strides, including the UB discussed.

To be clear, what you found was an actual bug in the implementation but also a product or trying to support the general case.

The choice is documented now.

Two conditions must hold: 1) the stride must divide the size of the array, 2) the stride sign must match the direction of the interval; otherwise, the result is undefined.

[correaa]

• Uninitialized elements still happen by default.
  The library has gained a trait to treat std::complex
  as trivial:

The trait is there for any class you can take responsibility for, including the possible ODR in C++17, etc.
This is an engineering decision, due to the unacceptable consequences of the cost of initializing values on the GPU and therefore in generic code.

This design makes the library avoid workarounds like std::make_unique_for_overwrite for types that natively should support it.

To be clear, uninitialized (partially formed) only happens in types that can afford it.

It is clearly documented in the section "Uninitialized vs. Initialized Elements", and you can opt out in many ways, including using a) a default element, b) empty brackets for default constructible, or you can opt in explicitly with the tag multi::uninitialized_elements.
In all cases, the library will never accidentally leave non-trivial types uninitialized, unless you force it with multi::force_element_trivial_default_construction.

Regarding force_element_trivial_default_construction, if you know another way to do it, please let me know. I didn't find another way, and I suspect there isn't because this is what std::ranges does with std::ranges::enable_borrowed_range, for example.

[correaa]

• The include/ directory still has lots of files that don't belong there.

The include directory has this:

~/boost-multi/include$ ls -all
total 8
drwxrwxr-x 1 correaa correaa   38 Jun 23 11:28 .
drwxrwxr-x 1 correaa correaa 1212 Aug  8 22:22 ..
drwxrwxr-x 1 correaa correaa   10 Jun 23 11:28 boost
lrwxrwxrwx 1 correaa correaa   11 Jun 23 11:28 multi -> boost/multi
-rw-rw-r-- 1 correaa correaa  362 Jun 23 11:28 multi.hpp

The include/boost/multi directory has this.
They all should be interface header files.

~/boost-multi/include/boost/multi$ ls -all
total 456
drwxrwxr-x 1 correaa correaa    190 Aug  8 22:07 .
drwxrwxr-x 1 correaa correaa     10 Jun 23 11:28 ..
drwxrwxr-x 1 correaa correaa    398 Jul 23 19:05 adaptors
drwxrwxr-x 1 correaa correaa     34 Jul 13 14:00 algorithms
-rw-rw-r-- 1 correaa correaa 120849 Aug  8 14:55 array.hpp
-rw-rw-r-- 1 correaa correaa 242306 Aug  8 22:07 array_ref.hpp
drwxrwxr-x 1 correaa correaa    400 Aug  8 12:14 detail
-rw-rw-r-- 1 correaa correaa  12806 Jul 22 13:41 elementwise.hpp
drwxrwxr-x 1 correaa correaa     20 Jun 23 11:28 io
-rw-rw-r-- 1 correaa correaa   3076 Aug  8 12:13 io.hpp
-rw-rw-r-- 1 correaa correaa  28692 Jul  9 13:16 restriction.hpp
-rw-rw-r-- 1 correaa correaa  42346 Jul  5 18:01 utility.hpp

[correaa]

• There are still many #if 0 preprocessor blocks.

All #if 0 blocks in headers have been removed; some in tests have been deleted or converted to conventional comments.

[correaa]

• The NOEXCEPT_ASSIGNMENT macro is still there.

NOEXCEPT_ASSIGNMENT has been removed

[correaa]

• The min and max functions are still unguarded.

Again, I completely missed Ruben's previous review.
There was no intention to resist the recommendations (most of which I would have accepted)

All mins and maxs have been protected (std::min) (std::max).

[correaa]

• Many tests are still missing from Jamfile.

33 missing tests have been added to the Jamfile

[correaa]

• The dynamic_array_cast leftover is still there.

The utilization of stray dynamic_array_cast has been removed.