[libcxx-commits] [PATCH] D154367: [libc++] mdspan - implement mdspan class

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__mdspan/mdspan.h:58-70
+// Helper for lightweight test checking that one didn't pass mapping instead of layout to mdspan
+namespace __mdspan_detail {
+template <class _LayoutPolicy>
+_LIBCPP_HIDE_FROM_ABI constexpr bool __is_possibly_mapping() {
+  return false;
+};
+
----------------
This can yield false positives if a type is both a mapping and a layout. It can actually lead to hard compiler errors if instantiating the nested types failed in a non SFINAE friendly way. This is a bit pedantic, but I'd be a lot more comfortable if we only restricted this to catching types that we know are *definitely* mappings, i.e. the mappings we provide. We could do that by defining the `__libcxx_is_mapping` nested type inside our mappings, and then checking:

```
template <class _Tp>
concept __is_definitely_mapping = requires { typename _Tp::__libcxx_is_mapping; };
```

Then we assert `!__is_definitely_mapping<_LayoutPolicy>` instead of `!__is_possibly_mapping<_LayoutPolicy>`. If this isn't harsh enough of a diagnostic cause it misses user-defined policies, I'd like to see this as an incremental change later, but not as part of the initial design of this class.

BTW we have a precedent for this with `__is_join_view_iterator`.


Edit:

Instead, I think we could do this too:

```
template <class _Layout, class _Extents>
concept __has_invalid_mapping = requires !requires { typename _Layout::template mapping<_Extents>; };
```

Then from within `mdspan`, you could do:

```
static_assert(!__has_invalid_mapping<_LayoutPolicy>,
    "mdspan: did you pass a valid LayoutPolicy to mdspan? A common mistake is passing a mapping instead of a layout policy");
```

WDYT? This keeps the guard rails for user-defined layouts and mappings as well. And there are no possibilities for false positives AFAICT since `_Layout::template mapping<_Extents>` *has* to be valid.


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

https://reviews.llvm.org/D154367