[libcxx-commits] [PATCH] D148067: [libcxx] Add mdspan/extents

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM w/ comments and CI. If some comments can't be applied, please LMK.

Thanks a lot for the contribution and for all the time you devoted during our live reviews!



================
Comment at: libcxx/include/__mdspan/extents.h:145
+  _LIBCPP_HIDE_FROM_ABI static constexpr _DynamicValues __zeros(std::index_sequence<Indices...>) noexcept {
+    return _DynamicValues{__zero(Indices)...};
+  }
----------------
crtrott wrote:
> crtrott wrote:
> > ldionne wrote:
> > > Then you can get rid of `__zero` above.
> > Turns out this warns: 
> > 
> > ```
> > llvm-project/build3/include/c++/v1/__mdspan/extents.h:143:28: error: left operand of comma operator has no effect [-Werror,-Wunused-value]
> >     return _DynamicValues{(Indices, 0)...};
> > ```
> > 
> n
```
return _DynamicValues{((void)Indices, 0)...};
```

This gets rid of the warning, it's a common trick.



================
Comment at: libcxx/include/__mdspan/extents.h:109
+struct __static_partial_sums {
+  static constexpr __possibly_empty_array<size_t, sizeof...(_Values)> __result{
+      __static_partial_sums_impl<_Values...>()};
----------------
Optional: If you want you could maybe use an immediately invoked lambda here to avoid giving a name to `__static_partial_sums_impl`, which is obviously just a one-shot helper for now:

```
static constexpr std::array<size_t, sizeof...(_Values)> __result = []{
  // implementation here
}();
```


================
Comment at: libcxx/include/__mdspan/extents.h:175-190
+template <size_t _Idx, size_t _FirstVal>
+struct __index_sequence_scan_impl<_Idx, _FirstVal> {
+#  if defined(__NVCC__) || defined(__NVCOMPILER)
+  // NVCC warns about pointless comparison with 0 for _Idx==0 and r being const
+  // evaluatable and also 0.
+  _LIBCPP_HIDE_FROM_ABI constexpr static size_t get(size_t __r) {
+    return static_cast<int64_t>(_Idx) > static_cast<int64_t>(__r) ? _FirstVal : 0;
----------------
crtrott wrote:
> ldionne wrote:
> > crtrott wrote:
> > > ldionne wrote:
> > > > I think we can simplify this one as well for a bit more compile-time win (https://godbolt.org/z/qjn471871):
> > > > 
> > > > ```
> > > > template <std::size_t ..._Values>
> > > > _LIBCPP_HIDE_FROM_ABI constexpr array<size_t, sizeof...(_Values)> __partial_sums_impl() {
> > > >   array<size_t, sizeof...(_Values)> __values = {_Values...};
> > > >   array<size_t, sizeof...(_Values)> __partial_sums;
> > > >   size_t __running_sum = 0;
> > > >   for (int __i = 0; __i != __values.size(); ++__i) {
> > > >     __partial_sums[__i] = __running_sum;
> > > >     __running_sum += __values[__i];
> > > >   }
> > > >   return __partial_sums;
> > > > }
> > > > 
> > > > template <size_t ..._Values>
> > > > struct __partial_sums {
> > > >   static constexpr array<size_t, sizeof...(_Values)> __result = __partial_sums_impl<_Values...>();
> > > > 
> > > >   _LIBCPP_HIDE_FROM_ABI constexpr static size_t __get(size_t __index) {
> > > >     return __result[__index];
> > > >   }
> > > > };
> > > > ```
> > > > 
> > > Turns out I run into issues for rank-0 extents. However, using __possibly_empty_array instead of std::array here makes it work. https://godbolt.org/z/1Tona7Mzz
> > I just investigated this a bit more. Here's the difference: https://godbolt.org/z/8zsMfzEd9.
> > 
> > The problem is that you had
> > 
> > ```
> > std::array<std::size_t, sizeof...(_Values)> __partial_sums;
> > ```
> > 
> > Instead, if you switch to
> > 
> > ```
> > std::array<std::size_t, sizeof...(_Values)> __partial_sums{};
> > ```
> > 
> > then your array is value-initialized and it works.
> Ah, makes sense. That said, do you feel we should move back to array instead of possibly_empty_array? I am inclined to leave it as is. If nothing else __possibly_empty_array is a bit simpler than std::array, and thus at the margin it may ease compile time a bit. Thoughts?
I think I would go for `std::array` cause it's more idiomatic and you don't need the properties of `maybe_empty_array` here. Otherwise it seems like we require the `maybe_empty_array` when we don't, which is a bit confusing.


================
Comment at: libcxx/test/std/containers/views/mdspan/extents/ConvertibleToIntegral.h:22
+
+#endif
----------------
Nit for consistency: `// TEST_STD_CONTAINERS_CONVERTIBLE_TO_INTEGRAL_H`


================
Comment at: libcxx/test/std/containers/views/mdspan/extents/comparison.pass.cpp:10-32
+// <mdspan>
+
+// template<class OtherIndexType, size_t... OtherExtents>
+//     friend constexpr bool operator==(const extents&,
+//                                      const extents<OtherIndexType, OtherExtents...>&) noexcept;
+//                                      `
+
----------------
There seems to be a copy-paste issue here.


================
Comment at: libcxx/test/std/containers/views/mdspan/extents/comparison.pass.cpp:38
+  assert((dest == src) == equal);
+  assert((dest != src) != equal);
+}
----------------
Equivalent, but semantically you want to check what the result of `!=` *is*, not what it *isn't*.


================
Comment at: libcxx/test/std/containers/views/mdspan/extents/ctor_default.pass.cpp:30-31
+  static constexpr void test_construction(AllExtents all_ext, Extents, std::index_sequence<Indices...>) {
+    // this function gets called twice: once with Extents being just dynamic ones, and once with all
+    // we only test during the all extent case, since then Indices is correct number
+    if constexpr (sizeof...(Indices) == E::rank()) {
----------------



================
Comment at: libcxx/test/std/containers/views/mdspan/extents/ctor_from_array.pass.cpp:79
+  // check that implicit construction doesn't work
+  LIBCPP_ASSERT((implicit_construction<std::extents<int, D, D, 5, D, D>>(a5).value == false));
+
----------------
I think you mean just a regular `assert` here, since this is not a libc++ extension.


================
Comment at: libcxx/test/std/containers/views/mdspan/extents/ctor_from_span.pass.cpp:60
+
+int main() {
+  test_index_type_combo<SpanCtorTest>();
----------------
Here's a really annoying one that we don't seem to have clang-tidy checks for. Please make sure you declare main as `int main(int, char**)` and make sure you `return 0;` at the end. This is needed for running tests in Freestanding, where `main()` is not a special function (so no implicit return and no implicit `extern "C"` mangling).

Please check throughout all your tests.


================
Comment at: libcxx/test/std/containers/views/mdspan/extents/ctor_from_span.pass.cpp:81
+  // check that implicit construction doesn't work
+  LIBCPP_ASSERT((implicit_construction<std::extents<int, D, D, 5, D, D>>(s5).value == false));
+
----------------
Same here, probably `assert`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148067/new/

https://reviews.llvm.org/D148067