kokkos / mdspan

Reference implementation of mdspan targeting C++23
Other
398 stars 65 forks source link

Replace spelled out logical operators: breaks Kokkos windows tests #344

Closed crtrott closed 2 months ago

crtrott commented 2 months ago

Errors such as:

D:\a\kokkos\kokkos\tpls\mdspan\include\experimental\__p0009_bits\utility.hpp(19): error : expected a "," [D:\a\kokkos\kokkos\build\core\src\kokkoscore.vcxproj]
      static_assert(std::is_integral<I1>::value and
masterleinad commented 2 months ago

Alternatively, you could do

diff --git a/include/experimental/__p0009_bits/macros.hpp b/include/experimental/__p0009_bits/macros.hpp
index 30209a6..34e2528 100644
--- a/include/experimental/__p0009_bits/macros.hpp
+++ b/include/experimental/__p0009_bits/macros.hpp
@@ -25,6 +25,10 @@
 #include "assert.h"
 #endif

+#ifdef _MSC_VER
+#include <iso646.h>
+#endif
+
 #ifndef _MDSPAN_HOST_DEVICE
 #  if defined(_MDSPAN_HAS_CUDA) || defined(_MDSPAN_HAS_HIP)
 #    define _MDSPAN_HOST_DEVICE __host__ __device__
crtrott commented 2 months ago

Let's avoid forcing default behavior change on downstream users.

mhoemmen commented 2 months ago

I was going to say that including <iso646.h> is the suggested option.

https://learn.microsoft.com/en-us/cpp/c-runtime-library/iso646-operators?view=msvc-170

The alternative spellings are required by the C++ Standard. Windows is nonconforming because it requires use of a header. That being said, I don't object to the proposed solution.

crtrott commented 2 months ago

Including that thing is transitive though, do we really want to change the default behavior of MSVC for everyone downstream?

mhoemmen commented 2 months ago

Including that thing is transitive though, do we really want to change the default behavior of MSVC for everyone downstream?

Just FYI, including that header doesn't "change the default behavior of MSVC," it makes MSVC conform to the C++ Standard a bit better. That being said, I don't object to the proposed solution.