search

kokkos / mdspan

Reference implementation of mdspan targeting C++23
Other
401 stars 66 forks source link

Workaround for missing <concepts> but __cpp_concepts is defined #274

Closed J-Meyers closed 1 year ago

J-Meyers commented 1 year ago

Fixes issue #273

This approach to fixing the issue avoids the include of \<concepts> entirely because the only thing needed from it was a very simple concept that could be recreated (same_as)

I put the new concept which is a duplicate of the standard one in the namespace internal rather than detail to avoid collisions with the standard library version which uses detail internally for its helper function __same_as

mhoemmen commented 1 year ago

@J-Meyers Thanks for the contribution! I have actually encountered the situation you're describing, where a compiler (specifically MSVC, I think with Visual Studio 2019) knows the concept and requires syntax, but doesn't have the <concepts> header.

Some questions:

  1. Does the compiler claim to implement C++20? If so, then a reasonable objection to this PR is "we already support C++17, so we don't need a special case for compilers that claim to support C++20 but don't."

  2. My experience with such versions of MSVC 2019 is that they don't have full support for the requires syntax. It works in some places, but not others. This means that supporting this use case is fragile.

  3. The concepts library paper P0898R3 proposes the feature test macro __cpp_lib_concepts for testing whether the concepts library is available. (~There's unfortunately no feature test macro for "the compiler supports concepts and constraints syntax."~ CORRECTION: __cpp_concepts is that feature test macro; see https://en.cppreference.com/w/cpp/feature_test .) Would you consider using this macro to decide whether the <concepts> header is available? (If that doesn't work, then I would consider the compiler too broken to be worth supporting.)

J-Meyers commented 1 year ago

@mhoemmen

@J-Meyers Thanks for the contribution! I have actually encountered the situation you're describing, where a compiler (specifically MSVC, I think with Visual Studio 2019) knows the concept and requires syntax, but doesn't have the <concepts> header.

Some questions:

  1. Does the compiler claim to implement C++20? If so, then a reasonable objection to this PR is "we already support C++17, so we don't need a special case for compilers that claim to support C++20 but don't."

The compiler's feature macros are accurate, it has the macro defined for __cpp_concepts and does support them, but the standard library does not define __cpp_lib_concepts and does not have the corresponding header \<concepts>

  1. My experience with such versions of MSVC 2019 is that they don't have full support for the requires syntax. It works in some places, but not others. This means that supporting this use case is fragile.
  2. The concepts library paper P0898R3 proposes the feature test macro __cpp_lib_concepts for testing whether the concepts library is available. (~There's unfortunately no feature test macro for "the compiler supports concepts and constraints syntax."~ CORRECTION: __cpp_concepts is that feature test macro; see https://en.cppreference.com/w/cpp/feature_test .) Would you consider using this macro to decide whether the <concepts> header is available? (If that doesn't work, then I would consider the compiler too broken to be worth supporting.)

That makes sense, I committed what you suggested on 60f9138 I came to nearly the same design, I hadn't seen your comment https://github.com/kokkos/mdspan/pull/274#discussion_r1223303873 when I made my changes so the only difference is the spacing around the ! defined where I didn't have the space between the ! and the defined which I found consistent with the style in the rest of the project, but I can change it for emphasis if you think it would help.

mhoemmen commented 1 year ago

I think our comments have overlapped in time a bit : - ) . Thanks for the explanations! I'll re-review.

mhoemmen commented 1 year ago

... so the only difference is the spacing around the ! defined where I didn't have the space between the ! and the defined which I found consistent with the style in the rest of the project, but I can change it for emphasis if you think it would help.

No worries -- you did the right thing by being consistent with the project's style : - ) . Thanks!!!