This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Add mdspan/extents
ClosedPublic

Authored by crtrott on Apr 11 2023, 9:20 PM.

Details

Reviewers
ldionne
dalg24
Group Reviewers
Restricted Project
Commits
rGfcaccf817d31: [libcxx] Add mdspan/extents
Summary

This patch adds std::extents. extents is one of the core classes used by std::mdspan. It describes a multi-dimensional index space with a mix of compile time and runtime sizes. Furthermore, it is templated on the index type used to describe the multi-dimensional index space.

The class is designed to be highly optimizable in performance critical code sections, and is fully useable in constant expressions contexts.

Testing of this class tends to be somewhat combinatorical, due to the large number of possible corner cases involved in situations where we have both runtime and compile time extents. To add to this, the class is designed to be interoperable (in particular constructible) from arguments which only need to be convertible to the index_type, but are otherwise arbitrary user types. For a larger discussion on the design of this class refer to: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p0009r18.html

Co-authored-by: Damien L-G <dalg24@gmail.com>

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
dalg24 updated this revision to Diff 513379.Apr 13 2023, 3:55 PM

More fiddling with the CI by Christian

dalg24 updated this revision to Diff 513392.Apr 13 2023, 5:11 PM

Fix module map

dalg24 updated this revision to Diff 513619.Apr 14 2023, 8:59 AM

Fiddling with header includes to fix modular tests and qualify std::size_t in tests

crtrott published this revision for review.Apr 17 2023, 6:21 PM

This is ready for review together with the child revision which fixes a few leftover issues flagged by the CI (missing updates in transitive include files, and ABI hiding macros).

Note the revision D148448 did pass the second stage CI testing (GCC 12, C++2b, Modules, C++11, C++03)

crtrott commandeered this revision.Apr 18 2023, 7:12 AM
crtrott added a reviewer: dalg24.

I will update this revision with fixes for CI

crtrott updated this revision to Diff 514637.Apr 18 2023, 7:13 AM

Fixed transitive includes, ABI hiding, and windows excluding assert test.

I have mainly looked at the libc++ style and not too deeply at the code itself. I saw @ldionne wanted to do a live review so I leave the detailed review to him.

libcxx/include/__mdspan/extents.h
414

Since the paper has been accepted for C++23 I would prefer to remove this comment and use the optimal version for C++23.

418

We typically only qualify function calls, since they maybe affected by ADL. There is a CI job that tests function calls are indeed qualified, much like the CI job that tests for _LIBCPP_HIDE_FROM_ABI.

470–477

I wonder whether the true branch should just be return false and reduce the scope of __value. LLVM has no single entry single exit requirement. WDYT?

482

Is this really in the Standard? Is it not automatically generated from operator== ?

If so, and you know why, can you add that information as comment?

libcxx/include/mdspan
24–31

We typically omit that for toplevel headers that only include their subheaders.

libcxx/test/libcxx/clang_tidy.sh.cpp
142 ↗(On Diff #514637)

When starting a new feature we typically add its feature-test macro to
libcxx/utils/generate_feature_test_macro_components.py with a "unimplemented": True, entry.

Note after that change you need to regenerate the libc++ files.

libcxx/test/std/containers/views/mdspan/extents/comparison.pass.cpp
16

If operator!= is not generated by the compiler can you add it here too?

libcxx/test/std/containers/views/mdspan/extents/cons.fail.cpp
56

Then (void)e1 is no longer needed.

libcxx/test/std/containers/views/mdspan/extents/constexpr_usage.pass.cpp
155

Is this test the same as the previous but only testing the constant evaluation?

If so we typically use a different pattern

constexpr bool test(){
  // test stuff

  return true;
}

int main(int const char**) {
  test();
  static_assert(test());
}

For example look at libcxx/test/std/utilities/format/format.syn/format_to_n_result.pass.cpp.

crtrott marked an inline comment as done.Apr 19 2023, 7:43 PM
crtrott added inline comments.
libcxx/include/__mdspan/extents.h
414

Yeah we can do that.

418

ok will remove.

470–477

Essentially we had issues with NVC++ and NVCC which would warn about missing return statements if you just return inside the if constexpr clauses.

482

Yeah actually we can remove this. C++20 onwards doesn't require it.

libcxx/include/mdspan
24–31

Will do

libcxx/test/libcxx/clang_tidy.sh.cpp
142 ↗(On Diff #514637)

will do

libcxx/test/std/containers/views/mdspan/extents/comparison.pass.cpp
16

It is.

libcxx/test/std/containers/views/mdspan/extents/cons.fail.cpp
56

will do

libcxx/test/std/containers/views/mdspan/extents/constexpr_usage.pass.cpp
155

Its not quite the same. Specifically the other test will assert individually on the extents paramemters matching, while this one just gives a grand total. So you wouldn't get an immediate feedback which thing exactly went wrong

crtrott updated this revision to Diff 515186.Apr 19 2023, 7:47 PM

Addressed Mordante's review comments:

  • removed C++14 back port code path
  • removed unnecessary class namespace qualification
  • removed unnecessary operator !=
  • removed unnecessary macro stuff in mdspan header
  • added feature test macro
  • used [[maybe_unused]] instead of (void)
crtrott updated this revision to Diff 515207.Apr 19 2023, 8:58 PM

Add missing generated file

ldionne requested changes to this revision.Apr 21 2023, 9:46 AM
ldionne added a subscriber: philnik.

Thank you so much for the patch! Leaving my comments from our live review.

libcxx/include/__mdspan/extents.h
21

This should go in <mdspan> instead. Note that we're discussing whether we even want to keep writing synopsis, but for the time being let's assume we will and do it consistently with the other headers.

148
149

I think this is equivalent (https://godbolt.org/z/8K9ds9Tej):

template <class _Tp, _Tp... _Values>
struct __static_array {
  static constexpr array<_Tp, sizeof...(_Values)> __array = {_Values...};

public:
  _LIBCPP_HIDE_FROM_ABI constexpr static size_t __size() { return sizeof...(_Values); }
  _LIBCPP_HIDE_FROM_ABI constexpr static _Tp __get(size_t __index) noexcept { return __array[__index]; }

  template <size_t _Index>
  _LIBCPP_HIDE_FROM_ABI constexpr static _Tp __get() { return __get(_Index); }
};

It's a bit simpler and should result in fewer instantiations.

176–191

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];
  }
};
202
205

Throughout?

211–212

I think that expresses your intent better, since this is really never called.

232

Or something else that doesn't have _t as a suffix. I'm not strongly attached, but we try not to use _t for typedefs (you can find some places in our code where we don't follow that, of course).

247–249

This is a workaround for the 0 case, and I don't think it's needed. What's the issue you are seeing if you remove the workaround? I would guess it's only a GCC issue since you mentioned it was some sort of tautological-comparison warning?

257

At the end of the day, we have the following constructors:

  1. Variadic / only initialize dynamic extents
  2. Variadic / initialize everything
  3. std::span / only initialize dynamic extents
  4. std::span / initialize everything

For this implementation-detail class, I would introduce two tag types to make it very obvious what's going on:

struct __initialize_dynamic_extents_t { };
struct __initialize_all_extents_t { };

 template <class ..._Vals>
_LIBCPP_HIDE_FROM_ABI constexpr __maybe_static_array(__initialize_dynamic_extents_t , Vals ...__vals)
      : __dyn_vals_{static_cast<_TDynamic>(__vals)...} {}

 template <class ..._Vals>
_LIBCPP_HIDE_FROM_ABI constexpr __maybe_static_array(__initialize_all_extents_t , Vals ...__vals) {
    static_assert((sizeof...(_DynVals) == __size_), "Invalid number of values.");
    _TDynamic __values[__size_]{static_cast<_TDynamic>(__vals)...};
    // etc...
}

// and then the same for the `std::span` version

In particular, I would remove the requires from these constructors here and go for more explicit and dumber constructors. The requires can then be left on the actual public APIs in std::extents below. From std::extents, you'd then use:

template <class... _OtherIndexTypes>
    requires(existing-logic-from-the-standard-and-nothing-more)
_LIBCPP_HIDE_FROM_ABI constexpr explicit extents(_OtherIndexTypes... __dynvals) noexcept
      : __vals_(conditional_t<sizeof...(_OtherIndexTypes) == __size_dynamic_, __initialize_dynamic_extents_t, __initialize_all_extents_t>{},
             static_cast<index_type>(__dynvals)...) {}

Suggestion: Try to remove the workaround for the 0 case above, and then let's see whether the contents of the requires are still hard to parse. So maybe don't do anything about this issue for now.

367–368

Throughout: we use static constexpr instead of constexpr static. It's a nitpick, but let's do it for consistency.

@philnik In case you are looking to add more clang-tidy checks, maybe that's one we could have.

371

Similarly here, I guess.

376

Please make sure that you have tests that check ASSERT_NOEXCEPT for all these methods. If you have some, just verify and mark the comment as done!

393

I would like for the preconditions in https://eel.is/c++draft/views.multidim#mdspan.extents.cons-7 (and friends) to be checked. Furthermore, and you might find it a bit nitpicky, please add a assert.foo.pass.cpp test that confirms that we crash when the preconditions are violated.

396–401

Then we can have only two constructors in the implementation-detail constructors: one with span, and a variadic one.

408

We shouldn't be std::moveing here.

412

I think there's a way to do this without recursion and without requiring multiple loops initializing the dynamic values, but I cant see it right now. Leaving a note to look again.

libcxx/test/std/containers/views/mdspan/extents/ConvertibleToIntegral.h
10

Let's make this TEST_STD_CONTAINERS_xxxxxxx etc.

libcxx/test/std/containers/views/mdspan/extents/assert.cons.pass.cpp
17–20 ↗(On Diff #515207)

These remarks are not necessary here IMO, but I would probably list the preconditions you're testing somewhere.

libcxx/test/std/containers/views/mdspan/extents/comparison.pass.cpp
81

We need to test all of this at constexpr and non-constexpr. You can see how we generally do this here, it's pretty easy: libcxx/test/std/strings/basic.string/string.cons/move.pass.cpp.

libcxx/test/std/containers/views/mdspan/extents/cons.fail.cpp
2

This should be moved to the cons.pass.cpp test, and we should verify that those ctors SFINAE away using requires instead. For example, something like this:

template <class ...Args>
concept ExtentsConstructible = requires (Args ...args) {
  std::extents<????>(args...);
};

static_assert(!ExtentsConstructible<...>);

Or you could just test !std::is_constructible<std::extents<...>, ...>, we do that in a bunch of places.

Rationale: these clang-verify tests are more fragile to diagnostic wording changing, and we really want to allow checking that on all supported compilers, not only Clang. Also, it allows other stdlibs to benefit from these tests.

libcxx/test/std/containers/views/mdspan/extents/cons.pass.cpp
2

Let's rename this to ctor.pass.cpp.

13

Please make sure you test one overload per .pass.cpp file. You can use a header file in this subdirectory to avoid creating a bunch of duplication.

libcxx/test/std/containers/views/mdspan/extents/constexpr_usage.pass.cpp
155

I would instead make sure that all the other tests are checked in constexpr and non-constexpr, and then drop this test.

libcxx/test/std/containers/views/mdspan/extents/conversion.fail.cpp
2

Same here for SFINAE.

This revision now requires changes to proceed.Apr 21 2023, 9:46 AM
ldionne added a reviewer: Restricted Project.Apr 21 2023, 9:47 AM
crtrott updated this revision to Diff 517562.EditedApr 27 2023, 8:16 AM

This implements feedback from review by @ldionne

Changes to extents.h

  • add check for extents parameter being representable as index_type
    • adds both the mandate for the template parameters, and precondition checks for constructors
  • add precondition check on extent/static_extent for r being < rank()
  • use simpler static_array impl
  • use simpler implementation for static scan, now called static_partial_sums
  • simplify constructors for __maybe_static_array
    • removed from array constructors (i.e. extents passes on a span)
    • implement zero length case via if constexpr in ctor body instead of different overloads
  • rename a few variables based on comments (i, r, static_vals_t, dyn_map_t, _Num)
  • fix order for constexpr static
  • move synopsis to mdspan header
  • fix default constructor not zero initializing dynamic values
    • this required a few little helper things to zero init __possibly_empty_array

Changes to tests

  • rework constructor tests to test one overload per cpp
    • use header file for common code to implement combinatorial testing
  • add test for default constructor
  • rewrite tests for non-allowed construction to use traits instead of failing compile
  • rewrite conversion tests to use traits to check for illegal conversions instead of fail test
  • add assert tests to test for all possible precondition violations
  • update comment in tests to have the definition from the standard in each header for what gets tested.
tschuett added inline comments.Apr 27 2023, 8:23 AM
libcxx/include/__mdspan/extents.h
13

Disclaimer: I am not a lawyer. The copyright notice might confuse vendors shipping libc++ or stop them to.

crtrott marked 22 inline comments as done.Apr 27 2023, 8:30 AM

Ticket off a bunch of comments as done. Looks like the only outstanding comment is that some tests don't test constexpr yet (e.g. comparison).

libcxx/include/__mdspan/extents.h
176–191

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

211–212

I used __libcpp_unreachable looked like that was the common practice in these headers.

257

I removed the from array constructor, implemented the zero case via constexpr if in the body instead of different overloads

crtrott marked 3 inline comments as done.Apr 27 2023, 8:45 AM
crtrott added inline comments.
libcxx/include/__mdspan/extents.h
13

I believe this is ok. The LLVM guidelines explicitly state that copyright is held by contributors.

The LLVM project does not collect copyright assignments, which means that the copyright for the code in the project is held by the respective contributors. Because you (or your company) retain ownership of the code you contribute, you know it may only be used under the terms of the open source license you contributed it under: the license for your contributions cannot be changed in the future without your approval.

Because the LLVM project does not require copyright assignments, changing the LLVM license requires tracking down the contributors to LLVM and getting them to agree that a license change is acceptable for their contributions. We feel that a high burden for relicensing is good for the project, because contributors do not have to fear that their code will be used in a way with which they disagree.

My understanding of the llvm exceptions in the License is that this copyright notice does not need to be reproduced by folks shipping compiled stuff. And if they ship the source they distribute this copyright notice implicitly so its fine. Note the original code where this Copyright is coming from is licensed under the exact same terms, including the LLVM exceptions.

Note also that LIBCXX already contains a bunch of Copyright notices. There are a whole lot from Microsoft which even say "All rights reserved" not just "certain rights", and Copyright s from Unicode, Inc. and a few individual folks.

crtrott updated this revision to Diff 517590.Apr 27 2023, 8:55 AM

Screwed up rebase before, so the patch didn't apply cleanly.

crtrott updated this revision to Diff 517776.Apr 27 2023, 7:49 PM
  • fix the two stage-2 CI issues observed (missing include, and unused variable)
  • make last few tests which weren't yet test constexpr too
    • that found one more constexpr needed inside extents itself
crtrott marked 5 inline comments as done.Apr 27 2023, 7:54 PM

Looks like we did address all but one comment for the header guard ...

crtrott updated this revision to Diff 517913.Apr 28 2023, 7:21 AM
  • update header guard
  • update Cxx2bPapers status (mark P0009 mdspan in progress)
  • hopefully fix module build
crtrott marked an inline comment as done.Apr 28 2023, 7:22 AM
crtrott updated this revision to Diff 517916.Apr 28 2023, 7:28 AM

Fix clang-format.

crtrott updated this revision to Diff 517922.Apr 28 2023, 7:48 AM

Rebase for transitive includes.

crtrott updated this revision to Diff 517946.Apr 28 2023, 8:47 AM

Attempt to fix module based build and transitive includes

I did another pass over this patch and I have no additional comments.
So no objections from my side to approve this patch when you're happy @ldionne.

crtrott updated this revision to Diff 518367.Apr 30 2023, 4:43 PM

Do not test for is_empty on Windows. Looks like that doesn't work there.

H-G-Hristov added inline comments.
libcxx/CREDITS.TXT
195 ↗(On Diff #518367)

The list is sorted by surname and formatted to allow easy grepping and

It seems you should reorder the list.

This basically LGTM. I'll take one last look when the comments have been applied and we can ship this. Thanks for the awesome patch!

libcxx/include/__mdspan/extents.h
137

I think I would use _LIBCPP_NO_UNIQUE_ADDRESS here instead.

146

Then you can get rid of __zero above.

173
226
235

std:: qualification here is not necessary, same for index_sequence in a few places.

236

I think that's what you mean.

243
253
255

Since this is not a function, full qualification is not needed. But you want to ADL-proof the calls so you do need one level of qualification. It should look like __mdspan_detail::__is_representable_as<_To>.

261

x2

307

Suggestion: constructors could come first like they do in the spec and like we usually do.

388

Can you confirm that this is optimized away by the compiler by default (assertions are disabled by default)? This should expand to

for (...) {
  if constexpr (make-sure-this-is-true) {
    __builtin_assume(is_representable_as(...));
  }
}

Hopefully this gets optimized out, otherwise I think we have a Clang bug on our hand.

403–410

IMO this is more readable:

using _CommonType = <...>; // nice little comment here explaining why we need this
if constexpr (rank() != sizeof...(_OtherExtents)) {
  return false;
} else {
  for (rank_type __r = 0; __r < __rank_; __r++) {
    if (static_cast<_CommonType>(__lhs.extent(__r)) != static_cast<_CommonType>(__rhs.extent(__r)))
      return false;
  }
}
return true;
libcxx/include/mdspan
48–52

Those are not in this review, right? If so, let's remove them from here.

72–73

I don't think those should be required. You might need to export size_t and span from the mdspan module to get rid of your modular CI issues if that was the problem you were trying to solve.

libcxx/test/std/containers/views/mdspan/extents/CtorTestCombinations.h
32–37 ↗(On Diff #518367)

You can just assert(ext.extent(r) == static_cast<typename E::index_type>(expected[r])) here and return void from this function, no?

68–94 ↗(On Diff #518367)

Here you can just directly call all the test_construction<...> functions without checking for a result.

98 ↗(On Diff #518367)

This guy needs to just return true unconditionally so you can call it inside a constexpr context.

libcxx/test/std/containers/views/mdspan/extents/ctor_default.pass.cpp
28 ↗(On Diff #518367)

I think this should be renamed?

35 ↗(On Diff #518367)
libcxx/test/std/containers/views/mdspan/extents/ctor_from_array.pass.cpp
40 ↗(On Diff #518367)

Probably needs a new name too. Please check throughout, I think this is copy/paste.

45–46 ↗(On Diff #518367)

You don't need to return anything from here too.

libcxx/test/std/containers/views/mdspan/extents/ctor_non_constructible.pass.cpp
56–59 ↗(On Diff #518367)

I would move these tests to their corresponding ctor_<wahtever>.pass.cpp file. We generally have these tests alongside our other runtime tests, since they are not really different from normal tests in any way.

libcxx/test/std/containers/views/mdspan/extents/dextents.pass.cpp
15 ↗(On Diff #518367)

Consider breaking this line.

libcxx/test/std/containers/views/mdspan/extents/types.pass.cpp
47

Here you want to use LIBCPP_STATIC_ASSERT instead because this is libc++ specific behavior, not something mandated by the standard.

crtrott updated this revision to Diff 518927.May 2 2023, 6:03 PM

Address more review comments

  • drop unnecessary std qualitication
  • update comparison to do early return
  • use _LIBCPP_NO_UNIQUE_ADDRESS
  • fixup numeric limits use in __is_representable_as
  • use common initializer list syntax
  • use integral concept to constrain types in representable_as helpers
  • use common_type for extent comparison
  • drop exposition-only helpers from synopsis
  • use simple assert instead bool pass-through returns in tests
  • fixup CREDITS.TXT alphabetical order
  • move non_constructible test into other ctor tests as appropriate
crtrott marked 21 inline comments as done.May 2 2023, 6:12 PM

Ticked off all the things we actually did, looks like there is a couple left over.

libcxx/include/__mdspan/extents.h
146

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)...};
146

n

307

We left it because it feels like the having the immediate accessors next to the variables is more readable.

412

Didn't come up with a way,

crtrott updated this revision to Diff 518936.May 2 2023, 7:01 PM
crtrott marked 5 inline comments as done.

Address last couple comments:

  • remove more exposition only from synopsis
  • break comment line
  • use LIBCPP_STATIC_ASSERT for is_empty check
  • remove stray std:: in extents.h
crtrott marked 4 inline comments as done.May 2 2023, 7:03 PM

Marked of remaining comments/questions.

libcxx/include/__mdspan/extents.h
13

Discussed with @ldionne and he agreed this is fine.

388

Yeah this all compiles away see this godbolt link:
https://godbolt.org/z/h5rqEMbcP

If you take the -DNDEBUG away it compiles into some big chunk unless it figures out that nothing bad could possible happen.
All in all this behaves as expected.

Note: the default is that assertions are ON.

ldionne added inline comments.May 4 2023, 1:15 PM
libcxx/include/__mdspan/extents.h
176–191

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.

ldionne added inline comments.May 4 2023, 1:22 PM
libcxx/include/__mdspan/extents.h
388

I corrected the godbolt: https://godbolt.org/z/15cbEacW4

My question was specifically about whether _LIBCPP_ASSERT inside the loop was going to get optimized out because it expands to __builtin_assume, and the answer is yes, indeed. Thanks for checking!

crtrott marked an inline comment as done.May 4 2023, 1:45 PM
crtrott added inline comments.
libcxx/include/__mdspan/extents.h
176–191

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?

philnik set the repository for this revision to rG LLVM Github Monorepo.May 12 2023, 2:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2023, 2:23 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
ldionne accepted this revision.May 15 2023, 9:35 AM

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!

libcxx/include/__mdspan/extents.h
110

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
}();
146
return _DynamicValues{((void)Indices, 0)...};

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

176–191

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.

libcxx/test/std/containers/views/mdspan/extents/ConvertibleToIntegral.h
23

Nit for consistency: // TEST_STD_CONTAINERS_CONVERTIBLE_TO_INTEGRAL_H

libcxx/test/std/containers/views/mdspan/extents/comparison.pass.cpp
11–33

There seems to be a copy-paste issue here.

39

Equivalent, but semantically you want to check what the result of != *is*, not what it *isn't*.

libcxx/test/std/containers/views/mdspan/extents/ctor_default.pass.cpp
30–31 ↗(On Diff #518936)
libcxx/test/std/containers/views/mdspan/extents/ctor_from_array.pass.cpp
79 ↗(On Diff #518936)

I think you mean just a regular assert here, since this is not a libc++ extension.

libcxx/test/std/containers/views/mdspan/extents/ctor_from_span.pass.cpp
60 ↗(On Diff #518936)

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.

81 ↗(On Diff #518936)

Same here, probably assert.

This revision is now accepted and ready to land.May 15 2023, 9:35 AM
crtrott updated this revision to Diff 522246.May 15 2023, 10:04 AM

Addressed last review comments.

crtrott marked 11 inline comments as done.May 15 2023, 10:05 AM
philnik added inline comments.May 15 2023, 12:05 PM
libcxx/docs/Status/Cxx2bPapers.csv
54 ↗(On Diff #522246)

A note listing what is already implemented/what still needs to be implemented would be nice.

crtrott added inline comments.May 15 2023, 5:32 PM
libcxx/docs/Status/Cxx2bPapers.csv
54 ↗(On Diff #522246)

What do you think of a pinky promise I file a separate change request to update that note, I think @ldionne was otherwise ready to merge this today.

CI LGTM, I checked and the only failure is a fluke. I'll merge this.

libcxx/docs/Status/Cxx2bPapers.csv
54 ↗(On Diff #522246)

I'd be fine with that! I am indeed looking forward to shipping this large review.

philnik added inline comments.May 16 2023, 8:45 AM
libcxx/docs/Status/Cxx2bPapers.csv
54 ↗(On Diff #522246)

Also fine with me.

This revision was landed with ongoing or failed builds.May 16 2023, 2:31 PM
Closed by commit rGfcaccf817d31: [libcxx] Add mdspan/extents (authored by crtrott, committed by philnik). · Explain Why
This revision was automatically updated to reflect the committed changes.