This is an automated email from the ASF dual-hosted git repository.
apitrou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new ed19a92198 GH-40252: [C++] Make span SFINAE standards-conforming to
enable compilation with nvcc (#40253)
ed19a92198 is described below
commit ed19a921983a3783a0aa36059328e2da0f31ae6f
Author: Vyas Ramasubramani <[email protected]>
AuthorDate: Wed Mar 13 03:17:12 2024 -0700
GH-40252: [C++] Make span SFINAE standards-conforming to enable compilation
with nvcc (#40253)
### Rationale for this change
The current code uses an incomplete type in a SFINAE construct.
Closes #40252
### What changes are included in this PR?
This PR changes the way that the type is specified to avoid any UB.
### Are these changes tested?
I tested locally that code that originally failed to compile with nvcc now
compiles successfully. The same code also compiles under clang and gcc. This is
a minimal reproducer:
```
#include <arrow/api.h>
#include <vector>
int main() {
arrow::util::span<int> x{std::vector<int>{1, 2, 3}};
}
```
I did not include it here because it is a compile-time rather than runtime
issue, and because at present it only manifests on nvcc.
### Are there any user-facing changes?
No.
* GitHub Issue: #40252
Authored-by: Vyas Ramasubramani <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
---
cpp/src/arrow/util/span.h | 28 ++++++++++++++++++++++++++--
cpp/src/arrow/util/span_test.cc | 1 -
2 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/cpp/src/arrow/util/span.h b/cpp/src/arrow/util/span.h
index 4254fec75e..71cf9ed448 100644
--- a/cpp/src/arrow/util/span.h
+++ b/cpp/src/arrow/util/span.h
@@ -25,6 +25,31 @@
namespace arrow::util {
+template <class T>
+class span;
+
+// This trait is used to check if a type R can be used to construct a span<T>.
+// Specifically, it checks if std::data(R) and std::size(R) are valid
expressions
+// that may be passed to the span(T*, size_t) constructor. The reason this
trait
+// is needed rather than expressing this directly in the relevant span
constructor
+// is that this check requires instantiating span<T>, which would violate the
+// C++ standard if written directly in the constructor's enable_if clause
+// because span<T> is an incomplete type at that point. By defining this trait
+// instead, we add an extra level of indirection that lets us delay the
+// evaluation of the template until the first time the associated constructor
+// is actually called, at which point span<T> is a complete type.
+//
+// Note that most compilers do support the noncompliant construct, but nvcc
+// does not. See https://github.com/apache/arrow/issues/40252
+template <class T, class R, class Enable = void>
+struct ConstructibleFromDataAndSize : std::false_type {};
+
+template <class T, class R>
+struct ConstructibleFromDataAndSize<
+ span<T>, R,
+ std::void_t<decltype(span<T>{std::data(std::declval<R>()),
+ std::size(std::declval<R>())})>> :
std::true_type {};
+
/// std::span polyfill.
///
/// Does not support static extents.
@@ -58,8 +83,7 @@ writing code which would break when it is replaced by
std::span.)");
template <
typename R,
- typename DisableUnlessConstructibleFromDataAndSize =
- decltype(span<T>(std::data(std::declval<R>()),
std::size(std::declval<R>()))),
+ std::enable_if_t<ConstructibleFromDataAndSize<span<T>, R>::value, bool>
= true,
typename DisableUnlessSimilarTypes = std::enable_if_t<std::is_same_v<
std::decay_t<std::remove_pointer_t<decltype(std::data(std::declval<R>()))>>,
std::decay_t<T>>>>
diff --git a/cpp/src/arrow/util/span_test.cc b/cpp/src/arrow/util/span_test.cc
index fcbb49f71e..01460c2bd0 100644
--- a/cpp/src/arrow/util/span_test.cc
+++ b/cpp/src/arrow/util/span_test.cc
@@ -51,7 +51,6 @@ TEST(Span, Construction) {
static_assert(std::is_constructible_v<span<const int>, decltype(arr)&>);
static_assert(std::is_constructible_v<span<const int>,
decltype(const_arr)&>);
- static_assert(std::is_constructible_v<span<const int>, span<int>>);
static_assert(std::is_constructible_v<span<int>, std::vector<int>&>);
static_assert(!std::is_constructible_v<span<int>, const std::vector<int>&>);
