This is an archive of the discontinued LLVM Phabricator instance.

[libc++] mdspan - implement default_accessor
ClosedPublic

Authored by crtrott on Jun 27 2023, 8:15 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG20c6b9d451ca: [libc++][mdspan] Implement default_accessor
Summary

This implements default_accessor and its tests for mdspan.

Diff Detail

Event Timeline

crtrott requested review of this revision.Jun 27 2023, 8:15 PM
crtrott created this revision.
crtrott updated this revision to Diff 535249.Jun 27 2023, 11:48 PM

Fix include order.

crtrott updated this revision to Diff 535408.Jun 28 2023, 7:54 AM

Fix whitespace in comment in test

crtrott updated this revision to Diff 535457.Jun 28 2023, 10:19 AM

Fix more formatting stuff.

crtrott updated this revision to Diff 535463.Jun 28 2023, 10:39 AM

Fix module include complaint

crtrott updated this revision to Diff 535470.Jun 28 2023, 11:11 AM

Fix another missing include

crtrott updated this revision to Diff 535479.Jun 28 2023, 11:42 AM

Missing include for modules build in test.

ldionne requested changes to this revision.Jun 28 2023, 1:06 PM
ldionne added inline comments.
libcxx/test/std/containers/views/mdspan/MinimalElementType.h
19
24–40

Let's make this all work at compile-time instead (https://godbolt.org/z/EshfM1rzn):

#include <memory>

struct MinimalElementType {
  int val;
  constexpr MinimalElementType() = delete;
  constexpr MinimalElementType(const MinimalElementType&) = delete;
  constexpr explicit MinimalElementType(int v) noexcept : val(v) {}
  constexpr MinimalElementType& operator=(const MinimalElementType&) = delete;
};

// Helper class to create pointer of MinimalElementType
template<class T, size_t N>
struct ElementPool {
  constexpr ElementPool() {
    ptr_ = std::allocator<T>().allocate(N);
    for (int i = 0; i != N; ++i)
      std::construct_at(ptr_ + i, 42);
  }

  constexpr T* get_ptr() { return ptr_; }

  constexpr ~ElementPool() {
    for (int i = 0; i != N; ++i)
      std::destroy_at(ptr_ + i);
    std::allocator<T>().deallocate(ptr_, N);
  }

private:
  T* ptr_;
};

constexpr bool f() {
    ElementPool<MinimalElementType, 128> pool;
    MinimalElementType* p = pool.get_ptr();
    return true;
}

int main() {
    f();
    static_assert(f());
}
libcxx/test/std/containers/views/mdspan/default_accessor/access.pass.cpp
31

Everywhere!

34
41

This won't be true anymore, we can run these tests always.

libcxx/test/std/containers/views/mdspan/default_accessor/ctor.conversion.pass.cpp
30–34

I don't find that the _t typedefs help readability. I think we should remove them.

53

Maybe add some sort of base/derived case here?

libcxx/test/std/containers/views/mdspan/default_accessor/ctor.default.pass.cpp
28

Do we want to add static_assert(std::is_trivially_default_constructible)? This would test that this is = default.

libcxx/test/std/containers/views/mdspan/default_accessor/element_type.verify.cpp
8
libcxx/test/std/containers/views/mdspan/default_accessor/offset.pass.cpp
26

This function name is wrong. Probably happens elsewhere too.

27–28

This comment isn't super useful IMO

34
This revision now requires changes to proceed.Jun 28 2023, 1:06 PM
crtrott updated this revision to Diff 535563.Jun 28 2023, 4:41 PM

Address review comments

crtrott marked 12 inline comments as done.Jun 28 2023, 4:43 PM
crtrott set the repository for this revision to rG LLVM Github Monorepo.Jun 29 2023, 9:47 AM
crtrott updated this revision to Diff 536834.Jul 3 2023, 10:27 AM

Rebase on top of main.

crtrott updated this revision to Diff 536878.Jul 3 2023, 1:16 PM

Fix module map issue which crept in during rebase

ldionne accepted this revision.Jul 13 2023, 9:29 AM

Ship it!

(after a few comments and rebase and green CI)

Thanks!

libcxx/test/std/containers/views/mdspan/MinimalElementType.h
10–11

For the 3 macros within this file.

20

Let's be explicit about the fact that this isn't movable either. So delete move ctor and move assignment.

This revision is now accepted and ready to land.Jul 13 2023, 9:29 AM
crtrott updated this revision to Diff 540173.Jul 13 2023, 1:52 PM

Address last of Louis' comments and rebase.

crtrott marked 2 inline comments as done.Jul 13 2023, 1:52 PM
This revision was automatically updated to reflect the committed changes.