Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

configurable namespaces #253

Open
nmm0 opened this issue Apr 4, 2023 · 12 comments
Open

configurable namespaces #253

nmm0 opened this issue Apr 4, 2023 · 12 comments
Assignees

Comments

@nmm0
Copy link
Contributor

nmm0 commented Apr 4, 2023

@crtrott mentioned that we want to be able to configure the namespace that is used for mdspan. We have a situation where mdspan is used as a fragment of a standard library and as a standalone library.

For example, in Kokkos, we may have both a mdspan header provided by the compiler and this mdspan library and we would have to avoid collisions.

We should allow configuring namespaces for:

Description example default cmake setting
Already standardized std::mdspan std MDSPAN_STANDARD_NAMESPACE
Future standard draft N/A std::experimental MDSPAN_DRAFT_NAMESPACE
Proposed additions to the standard std::padded_layout* std::experimental MDSPAN_PROPOSED_NAMESPACE
Extensions (won't be proposed) N/A mdspan MDSPAN_EXTENSION_NAMESPACE

Please let me know if you have an opinions on these names

@nmm0 nmm0 self-assigned this Apr 4, 2023
@crtrott
Copy link
Member

crtrott commented Apr 5, 2023

One thing I wonder is whether we should make these namespaces implementation details?

@crtrott
Copy link
Member

crtrott commented Apr 5, 2023

Another question I have is whether something which got design approved for our purposes should be treated as MDSPAN_DRAFT_NAMESPACE, e.g. submdspan.

@crtrott
Copy link
Member

crtrott commented Apr 5, 2023

But yeah I think those 4 namespace categories and the names for the macros (+- an "IMPL") are good.

@crtrott
Copy link
Member

crtrott commented Apr 5, 2023

Examples of stuff we will not propose could be things like cuda_accessor or device_aware_simple_container<MemorySpace>

@mhoemmen
Copy link
Contributor

mhoemmen commented Apr 5, 2023

@crtrott wrote:

One thing I wonder is whether we should make these namespaces implementation details?

It's pretty likely some user will want to control those macros, as a way to work around some conflicts between packages.

@mhoemmen
Copy link
Contributor

mhoemmen commented Apr 5, 2023

@crtrott was concerned about not being able to define namespaces in a single line in C++ < 17. I do still recommend just assuming C++ >= 17. On the other hand, libcu++ deals with this issue with a few different macros. For example, in cuda/std/detail/__config, we have

// redefine namespace std::
#undef _LIBCUDACXX_BEGIN_NAMESPACE_STD
#define _LIBCUDACXX_BEGIN_NAMESPACE_STD \
   namespace cuda { namespace std { inline namespace _LIBCUDACXX_CUDA_ABI_NAMESPACE {

#undef _LIBCUDACXX_END_NAMESPACE_STD
#define _LIBCUDACXX_END_NAMESPACE_STD } } }

#undef _CUDA_VSTD
#define _CUDA_VSTD cuda::std::_LIBCUDACXX_CUDA_ABI_NAMESPACE

where _LIBCUDACXX_CUDA_ABI_NAMESPACE is a macro relating to the ABI version. The idea is that code uses _CUDA_VSTD to name the (possibly nested) namespace, and the other macros to begin or end the (possibly nested) namespace.

The reference implementation probably doesn't have to fuss about ABI versions, but libcxx likely does.

@nliber
Copy link
Contributor

nliber commented Apr 5, 2023

I don't know that either MDSPAN_DRAFT_NAMESPACE or MDSPAN_PROPOSED_NAMESPACE should be defaulted to std::experimental. There aren't going to be any more Library Fundamental TSes (after the current v3 is voted in) P2708.

@nliber
Copy link
Contributor

nliber commented Apr 5, 2023

I'm not sure if having MDSPAN_EXTENSION_NAMESPACE default to mdspan is a good idea (from the user perspective), because that is also the name of the class. At a minimum, it'll always require context to understand compiler error messages. Maybe mdspanext?

@crtrott
Copy link
Member

crtrott commented Apr 5, 2023

I mean we could go radical and say the defaults are not std something at all. And we can add a cmake option to change the set of namespaces to std::

@nmm0
Copy link
Contributor Author

nmm0 commented Apr 5, 2023

I mean we could go radical and say the defaults are not std something at all. And we can add a cmake option to change the set of namespaces to std::

I honestly wouldn't mind that. After all, we aren't actually shipping as a standard library directly. Most of our use case will be as a TPL for Kokkos, even if it is nice to have some examples in std for committee stuff

@mhoemmen
Copy link
Contributor

mhoemmen commented Apr 6, 2023

And we can add a cmake option to change the set of namespaces to std::

or just the following, perhaps?

#if ! defined(MDSPAN_PROPOSED_NAMESPACE)
#  define MDSPAN_PROPOSED_NAMESPACE whatever_default
#endif

@crtrott
Copy link
Member

crtrott commented Apr 7, 2023

Yeah I kinda was thinking that Mark, as the primary implementation thing. Its just the question whether we have a cmake option which defines those if you ask for it.

nmm0 added a commit to nmm0/mdspan that referenced this issue Apr 20, 2023
…it's not under <experimental/*> unless using 'std' mode
nmm0 added a commit to nmm0/mdspan that referenced this issue Apr 21, 2023
nmm0 added a commit to nmm0/mdspan that referenced this issue Apr 21, 2023
nmm0 added a commit to nmm0/mdspan that referenced this issue Apr 21, 2023
nmm0 added a commit to nmm0/mdspan that referenced this issue Apr 24, 2023
…pace name. Also add some helper macros to make this easier
nmm0 added a commit to nmm0/mdspan that referenced this issue Apr 24, 2023
nmm0 added a commit to nmm0/mdspan that referenced this issue Apr 24, 2023
…td`-like mode and `__cpp_lib_span` is defined. This prevents duplicate definitions with the standard library
nmm0 added a commit to nmm0/mdspan that referenced this issue May 1, 2023
…t instead. This avoids clashes when using the std headers
nmm0 added a commit to nmm0/mdspan that referenced this issue May 1, 2023
mhoemmen pushed a commit to mhoemmen/mdspan that referenced this issue Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants