You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "bkietz (via GitHub)" <gi...@apache.org> on 2023/06/27 20:52:53 UTC
[GitHub] [arrow] bkietz opened a new pull request, #36334: GH-15281: [C++] Replace bytes_view alias with span
bkietz opened a new pull request, #36334:
URL: https://github.com/apache/arrow/pull/36334
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
basic_string_view was never really intended to support types other than chars; the template which the STL intends for use in this situation is std::span
### What changes are included in this PR?
Since std::span is added in c++20, a minimal polyfill is added named util::span.
- doesn't include support for static extents
- some accessors like `front()` are not present in util::span
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36334:
URL: https://github.com/apache/arrow/pull/36334#issuecomment-1630238736
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a diff in pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #36334:
URL: https://github.com/apache/arrow/pull/36334#discussion_r1254603594
##########
cpp/src/arrow/util/span.h:
##########
@@ -0,0 +1,129 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+#include <cstring>
+#include <iterator>
+#include <type_traits>
+
+namespace arrow::util {
+
+/// std::span polyfill.
+///
+/// Does not support static extents.
+template <typename T>
+class span {
+ static_assert(sizeof(T),
+ R"(
+std::span allows contiguous_iterators instead of just pointers, the enforcement
+of which requires T to be a complete type. arrow::util::span does not support
+contiguous_iterators, but T is still required to be a complete type to prevent
+writing code which would break when it is replaced by std::span.)");
+
+ public:
+ using element_type = T;
+ using value_type = std::remove_cv_t<T>;
+ using iterator = T*;
+ using const_iterator = T const*;
+
+ span() = default;
+ span(const span&) = default;
+ span& operator=(const span&) = default;
+
+ template <typename M, typename = std::enable_if_t<std::is_same_v<T, M const>>>
+ // NOLINTNEXTLINE runtime/explicit
+ constexpr span(span<M> mut) : span{mut.data(), mut.size()} {}
+
+ constexpr span(T* data, size_t count) : data_{data}, size_{count} {}
+
+ constexpr span(T* begin, T* end)
+ : data_{begin}, size_{static_cast<size_t>(end - begin)} {}
+
+ template <
+ typename R,
+ typename DisableUnlessConstructibleFromDataAndSize =
+ decltype(span<T>(std::data(std::declval<R&>()), std::size(std::declval<R&>()))),
+ 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>>>>
+ // NOLINTNEXTLINE runtime/explicit, non-const reference
+ constexpr span(R& range) : span{std::data(range), std::size(range)} {}
Review Comment:
Alright, I'll add a test suite to check more construction cases against std::span
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36334:
URL: https://github.com/apache/arrow/pull/36334#issuecomment-1610200506
Should also close #29678.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a diff in pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #36334:
URL: https://github.com/apache/arrow/pull/36334#discussion_r1254603594
##########
cpp/src/arrow/util/span.h:
##########
@@ -0,0 +1,129 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+#include <cstring>
+#include <iterator>
+#include <type_traits>
+
+namespace arrow::util {
+
+/// std::span polyfill.
+///
+/// Does not support static extents.
+template <typename T>
+class span {
+ static_assert(sizeof(T),
+ R"(
+std::span allows contiguous_iterators instead of just pointers, the enforcement
+of which requires T to be a complete type. arrow::util::span does not support
+contiguous_iterators, but T is still required to be a complete type to prevent
+writing code which would break when it is replaced by std::span.)");
+
+ public:
+ using element_type = T;
+ using value_type = std::remove_cv_t<T>;
+ using iterator = T*;
+ using const_iterator = T const*;
+
+ span() = default;
+ span(const span&) = default;
+ span& operator=(const span&) = default;
+
+ template <typename M, typename = std::enable_if_t<std::is_same_v<T, M const>>>
+ // NOLINTNEXTLINE runtime/explicit
+ constexpr span(span<M> mut) : span{mut.data(), mut.size()} {}
+
+ constexpr span(T* data, size_t count) : data_{data}, size_{count} {}
+
+ constexpr span(T* begin, T* end)
+ : data_{begin}, size_{static_cast<size_t>(end - begin)} {}
+
+ template <
+ typename R,
+ typename DisableUnlessConstructibleFromDataAndSize =
+ decltype(span<T>(std::data(std::declval<R&>()), std::size(std::declval<R&>()))),
+ 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>>>>
+ // NOLINTNEXTLINE runtime/explicit, non-const reference
+ constexpr span(R& range) : span{std::data(range), std::size(range)} {}
Review Comment:
Alright, I'll add a more tests to check construction cases against std::span
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a diff in pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #36334:
URL: https://github.com/apache/arrow/pull/36334#discussion_r1258197945
##########
cpp/src/arrow/util/span.h:
##########
@@ -0,0 +1,129 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+#include <cstring>
+#include <iterator>
+#include <type_traits>
+
+namespace arrow::util {
+
+/// std::span polyfill.
+///
+/// Does not support static extents.
+template <typename T>
+class span {
+ static_assert(sizeof(T),
+ R"(
+std::span allows contiguous_iterators instead of just pointers, the enforcement
+of which requires T to be a complete type. arrow::util::span does not support
+contiguous_iterators, but T is still required to be a complete type to prevent
+writing code which would break when it is replaced by std::span.)");
+
+ public:
+ using element_type = T;
+ using value_type = std::remove_cv_t<T>;
+ using iterator = T*;
+ using const_iterator = T const*;
+
+ span() = default;
+ span(const span&) = default;
+ span& operator=(const span&) = default;
+
+ template <typename M, typename = std::enable_if_t<std::is_same_v<T, M const>>>
+ // NOLINTNEXTLINE runtime/explicit
+ constexpr span(span<M> mut) : span{mut.data(), mut.size()} {}
+
+ constexpr span(T* data, size_t count) : data_{data}, size_{count} {}
+
+ constexpr span(T* begin, T* end)
+ : data_{begin}, size_{static_cast<size_t>(end - begin)} {}
+
+ template <
+ typename R,
+ typename DisableUnlessConstructibleFromDataAndSize =
+ decltype(span<T>(std::data(std::declval<R>()), std::size(std::declval<R>()))),
+ 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>>>>
+ // NOLINTNEXTLINE runtime/explicit, non-const reference
+ constexpr span(R&& range) : span{std::data(range), std::size(range)} {}
+
+ constexpr T* begin() const { return data_; }
+ constexpr T* end() const { return data_ + size_; }
+ constexpr T* data() const { return data_; }
+
+ constexpr size_t size() const { return size_; }
+ constexpr size_t size_bytes() const { return size_ * sizeof(T); }
+ constexpr bool empty() const { return size_ == 0; }
+
+ constexpr T& operator[](size_t i) { return data_[i]; }
+ constexpr const T& operator[](size_t i) const { return data_[i]; }
+
+ constexpr span subspan(size_t offset) const {
+ if (offset > size_) return {data_, data_};
+ return {data_ + offset, size_ - offset};
+ }
+
+ constexpr span subspan(size_t offset, size_t count) const {
+ auto out = subspan(offset);
+ if (count < out.size_) {
+ out.size_ = count;
+ }
+ return out;
+ }
+
+ constexpr bool operator==(span const& other) const {
+ if (size_ != other.size_) return false;
+
+ if constexpr (std::is_integral_v<T>) {
+ return std::memcmp(data_, other.data_, size_bytes()) == 0;
Review Comment:
Good catch, I'll add an explicit check for `data_`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36334:
URL: https://github.com/apache/arrow/pull/36334#discussion_r1251173470
##########
cpp/src/arrow/util/span.h:
##########
@@ -0,0 +1,129 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+#include <cstring>
+#include <iterator>
+#include <type_traits>
+
+namespace arrow::util {
+
+/// std::span polyfill.
+///
+/// Does not support static extents.
+template <typename T>
+class span {
+ static_assert(sizeof(T),
+ R"(
+std::span allows contiguous_iterators instead of just pointers, the enforcement
+of which requires T to be a complete type. arrow::util::span does not support
+contiguous_iterators, but T is still required to be a complete type to prevent
+writing code which would break when it is replaced by std::span.)");
+
+ public:
+ using element_type = T;
+ using value_type = std::remove_cv_t<T>;
+ using iterator = T*;
+ using const_iterator = T const*;
+
+ span() = default;
+ span(const span&) = default;
+ span& operator=(const span&) = default;
+
+ template <typename M, typename = std::enable_if_t<std::is_same_v<T, M const>>>
+ // NOLINTNEXTLINE runtime/explicit
+ constexpr span(span<M> mut) : span{mut.data(), mut.size()} {}
+
+ constexpr span(T* data, size_t count) : data_{data}, size_{count} {}
+
+ constexpr span(T* begin, T* end)
+ : data_{begin}, size_{static_cast<size_t>(end - begin)} {}
+
+ template <
+ typename R,
+ typename DisableUnlessConstructibleFromDataAndSize =
+ decltype(span<T>(std::data(std::declval<R&>()), std::size(std::declval<R&>()))),
+ 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>>>>
+ // NOLINTNEXTLINE runtime/explicit, non-const reference
+ constexpr span(R& range) : span{std::data(range), std::size(range)} {}
Review Comment:
Should this be `R&&`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36334:
URL: https://github.com/apache/arrow/pull/36334#discussion_r1256488546
##########
cpp/src/arrow/util/span.h:
##########
@@ -0,0 +1,129 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+#include <cstring>
+#include <iterator>
+#include <type_traits>
+
+namespace arrow::util {
+
+/// std::span polyfill.
+///
+/// Does not support static extents.
+template <typename T>
+class span {
+ static_assert(sizeof(T),
+ R"(
+std::span allows contiguous_iterators instead of just pointers, the enforcement
+of which requires T to be a complete type. arrow::util::span does not support
+contiguous_iterators, but T is still required to be a complete type to prevent
+writing code which would break when it is replaced by std::span.)");
+
+ public:
+ using element_type = T;
+ using value_type = std::remove_cv_t<T>;
+ using iterator = T*;
+ using const_iterator = T const*;
+
+ span() = default;
+ span(const span&) = default;
+ span& operator=(const span&) = default;
+
+ template <typename M, typename = std::enable_if_t<std::is_same_v<T, M const>>>
+ // NOLINTNEXTLINE runtime/explicit
+ constexpr span(span<M> mut) : span{mut.data(), mut.size()} {}
+
+ constexpr span(T* data, size_t count) : data_{data}, size_{count} {}
+
+ constexpr span(T* begin, T* end)
+ : data_{begin}, size_{static_cast<size_t>(end - begin)} {}
+
+ template <
+ typename R,
+ typename DisableUnlessConstructibleFromDataAndSize =
+ decltype(span<T>(std::data(std::declval<R>()), std::size(std::declval<R>()))),
+ 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>>>>
+ // NOLINTNEXTLINE runtime/explicit, non-const reference
+ constexpr span(R&& range) : span{std::data(range), std::size(range)} {}
+
+ constexpr T* begin() const { return data_; }
+ constexpr T* end() const { return data_ + size_; }
+ constexpr T* data() const { return data_; }
+
+ constexpr size_t size() const { return size_; }
+ constexpr size_t size_bytes() const { return size_ * sizeof(T); }
+ constexpr bool empty() const { return size_ == 0; }
+
+ constexpr T& operator[](size_t i) { return data_[i]; }
+ constexpr const T& operator[](size_t i) const { return data_[i]; }
+
+ constexpr span subspan(size_t offset) const {
+ if (offset > size_) return {data_, data_};
+ return {data_ + offset, size_ - offset};
+ }
+
+ constexpr span subspan(size_t offset, size_t count) const {
+ auto out = subspan(offset);
+ if (count < out.size_) {
+ out.size_ = count;
+ }
+ return out;
+ }
+
+ constexpr bool operator==(span const& other) const {
+ if (size_ != other.size_) return false;
+
+ if constexpr (std::is_integral_v<T>) {
+ return std::memcmp(data_, other.data_, size_bytes()) == 0;
Review Comment:
Note that `memcmp` doesn't accept non-null pointers, even if size is 0. Perhaps we should guard against this (the fallback impl below doesn't have this problem AFAICT).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a diff in pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #36334:
URL: https://github.com/apache/arrow/pull/36334#discussion_r1258452117
##########
cpp/src/arrow/util/span.h:
##########
@@ -0,0 +1,129 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+#include <cstring>
+#include <iterator>
+#include <type_traits>
+
+namespace arrow::util {
+
+/// std::span polyfill.
+///
+/// Does not support static extents.
+template <typename T>
+class span {
+ static_assert(sizeof(T),
+ R"(
+std::span allows contiguous_iterators instead of just pointers, the enforcement
+of which requires T to be a complete type. arrow::util::span does not support
+contiguous_iterators, but T is still required to be a complete type to prevent
+writing code which would break when it is replaced by std::span.)");
+
+ public:
+ using element_type = T;
+ using value_type = std::remove_cv_t<T>;
+ using iterator = T*;
+ using const_iterator = T const*;
+
+ span() = default;
+ span(const span&) = default;
+ span& operator=(const span&) = default;
+
+ template <typename M, typename = std::enable_if_t<std::is_same_v<T, M const>>>
+ // NOLINTNEXTLINE runtime/explicit
+ constexpr span(span<M> mut) : span{mut.data(), mut.size()} {}
+
+ constexpr span(T* data, size_t count) : data_{data}, size_{count} {}
+
+ constexpr span(T* begin, T* end)
+ : data_{begin}, size_{static_cast<size_t>(end - begin)} {}
+
+ template <
+ typename R,
+ typename DisableUnlessConstructibleFromDataAndSize =
+ decltype(span<T>(std::data(std::declval<R>()), std::size(std::declval<R>()))),
+ 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>>>>
+ // NOLINTNEXTLINE runtime/explicit, non-const reference
+ constexpr span(R&& range) : span{std::data(range), std::size(range)} {}
+
+ constexpr T* begin() const { return data_; }
+ constexpr T* end() const { return data_ + size_; }
+ constexpr T* data() const { return data_; }
+
+ constexpr size_t size() const { return size_; }
+ constexpr size_t size_bytes() const { return size_ * sizeof(T); }
+ constexpr bool empty() const { return size_ == 0; }
+
+ constexpr T& operator[](size_t i) { return data_[i]; }
+ constexpr const T& operator[](size_t i) const { return data_[i]; }
+
+ constexpr span subspan(size_t offset) const {
+ if (offset > size_) return {data_, data_};
+ return {data_ + offset, size_ - offset};
+ }
+
+ constexpr span subspan(size_t offset, size_t count) const {
+ auto out = subspan(offset);
+ if (count < out.size_) {
+ out.size_ = count;
+ }
+ return out;
+ }
+
+ constexpr bool operator==(span const& other) const {
+ if (size_ != other.size_) return false;
+
+ if constexpr (std::is_integral_v<T>) {
+ return std::memcmp(data_, other.data_, size_bytes()) == 0;
Review Comment:
... I'm surprised the sanitizer didn't tell me about this one
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #36334:
URL: https://github.com/apache/arrow/pull/36334#discussion_r1246961185
##########
cpp/src/arrow/util/span.h:
##########
@@ -0,0 +1,124 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+#include <iterator>
+#include <type_traits>
+
+namespace arrow::util {
+
+/// std::span polyfill.
+///
+/// Does not support static extents.
+template <typename T>
+class span {
+ static_assert(sizeof(T),
+ R"(
+std::span allows contiguous_iterators instead of just pointers, the enforcement
+of which requires T to be a complete type. arrow::util::span does not support
+contiguous_iterators, but T is still required to be a complete type to prevent
+writing code which would break when it is replaced by std::span.)");
+
+ public:
+ using element_type = T;
+ using value_type = std::remove_cv_t<T>;
+ using iterator = T*;
+ using const_iterator = T const*;
+
+ span() = default;
+ span(const span&) = default;
+ span& operator=(const span&) = default;
+
+ template <typename M, typename = std::enable_if_t<std::is_same_v<T, M const>>>
+ // NOLINTNEXTLINE runtime/explicit
+ constexpr span(span<M> mut) : span{mut.data(), mut.size()} {}
+
+ constexpr span(T* data, size_t count) : data_{data}, size_{count} {}
+
+ constexpr span(T* begin, T* end)
+ : data_{begin}, size_{static_cast<size_t>(end - begin)} {}
+
+ template <
+ typename R,
+ typename DisableUnlessConstructibleFromDataAndSize =
+ decltype(span<T>(std::data(std::declval<R&>()), std::size(std::declval<R&>()))),
+ 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>>>>
+ // NOLINTNEXTLINE runtime/explicit, non-const reference
+ constexpr span(R& range) : span{std::data(range), std::size(range)} {}
+
+ constexpr T* begin() const { return data_; }
+ constexpr T* end() const { return data_ + size_; }
+ constexpr T* data() const { return data_; }
+
+ constexpr size_t size() const { return size_; }
+ constexpr size_t size_bytes() const { return size_ * sizeof(T); }
+ constexpr bool empty() const { return size_ == 0; }
+
+ constexpr T& operator[](size_t i) { return data_[i]; }
+ constexpr const T& operator[](size_t i) const { return data_[i]; }
+
+ constexpr span subspan(size_t offset) const {
+ if (offset > size_) return {};
+ return {data_ + offset, size_ - offset};
+ }
+
+ constexpr span subspan(size_t offset, size_t count) const {
+ auto out = subspan(offset);
+ if (count < out.size_) {
+ out.size_ = count;
+ }
+ return out;
+ }
+
+ constexpr bool operator==(span const& other) const {
+ if (size_ != other.size_) return false;
+
+ T* ptr = data_;
+ for (T const& e : other) {
+ if (*ptr++ != e) return false;
+ }
+ return true;
+ }
Review Comment:
Does the compiler turns this into `memcmp` when `T` is an integer type?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a diff in pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #36334:
URL: https://github.com/apache/arrow/pull/36334#discussion_r1246999183
##########
cpp/src/arrow/util/span.h:
##########
@@ -0,0 +1,124 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+#include <iterator>
+#include <type_traits>
+
+namespace arrow::util {
+
+/// std::span polyfill.
+///
+/// Does not support static extents.
+template <typename T>
+class span {
+ static_assert(sizeof(T),
+ R"(
+std::span allows contiguous_iterators instead of just pointers, the enforcement
+of which requires T to be a complete type. arrow::util::span does not support
+contiguous_iterators, but T is still required to be a complete type to prevent
+writing code which would break when it is replaced by std::span.)");
+
+ public:
+ using element_type = T;
+ using value_type = std::remove_cv_t<T>;
+ using iterator = T*;
+ using const_iterator = T const*;
+
+ span() = default;
+ span(const span&) = default;
+ span& operator=(const span&) = default;
+
+ template <typename M, typename = std::enable_if_t<std::is_same_v<T, M const>>>
+ // NOLINTNEXTLINE runtime/explicit
+ constexpr span(span<M> mut) : span{mut.data(), mut.size()} {}
+
+ constexpr span(T* data, size_t count) : data_{data}, size_{count} {}
+
+ constexpr span(T* begin, T* end)
+ : data_{begin}, size_{static_cast<size_t>(end - begin)} {}
+
+ template <
+ typename R,
+ typename DisableUnlessConstructibleFromDataAndSize =
+ decltype(span<T>(std::data(std::declval<R&>()), std::size(std::declval<R&>()))),
+ 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>>>>
+ // NOLINTNEXTLINE runtime/explicit, non-const reference
+ constexpr span(R& range) : span{std::data(range), std::size(range)} {}
+
+ constexpr T* begin() const { return data_; }
+ constexpr T* end() const { return data_ + size_; }
+ constexpr T* data() const { return data_; }
+
+ constexpr size_t size() const { return size_; }
+ constexpr size_t size_bytes() const { return size_ * sizeof(T); }
+ constexpr bool empty() const { return size_ == 0; }
+
+ constexpr T& operator[](size_t i) { return data_[i]; }
+ constexpr const T& operator[](size_t i) const { return data_[i]; }
+
+ constexpr span subspan(size_t offset) const {
+ if (offset > size_) return {};
+ return {data_ + offset, size_ - offset};
+ }
+
+ constexpr span subspan(size_t offset, size_t count) const {
+ auto out = subspan(offset);
+ if (count < out.size_) {
+ out.size_ = count;
+ }
+ return out;
+ }
+
+ constexpr bool operator==(span const& other) const {
+ if (size_ != other.size_) return false;
+
+ T* ptr = data_;
+ for (T const& e : other) {
+ if (*ptr++ != e) return false;
+ }
+ return true;
+ }
Review Comment:
I've played with this a bit and couldn't get the compiler to do it automatically. I'll use `if constexpr (std::is_integral_v<T>)` to dispatch to memcmp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36334:
URL: https://github.com/apache/arrow/pull/36334#issuecomment-1630307396
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36334:
URL: https://github.com/apache/arrow/pull/36334#issuecomment-1629527937
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36334:
URL: https://github.com/apache/arrow/pull/36334#issuecomment-1613194019
Let's not overengineer it. You just want to check the pointer/length of a span, so you can write a trivial helper function `AssertSpansEqual` (or `AssertSpansIdentical` if you want to stress that it's not a logical comparison of the underlying values).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36334:
URL: https://github.com/apache/arrow/pull/36334#issuecomment-1630241726
Revision: 9ce75a142734f8c675d0110bcf6ce51aadfb877e
Submitted crossbow builds: [ursacomputing/crossbow @ actions-035d06cfbf](https://github.com/ursacomputing/crossbow/branches/all?query=actions-035d06cfbf)
|Task|Status|
|----|------|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5516703343/jobs/10058368676)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/5516701207/jobs/10058364066)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5516701701/jobs/10058365329)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/runs/14935270411)|
|test-cuda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5516701261/jobs/10058364354)|
|test-debian-11-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5516701503/jobs/10058364759)|
|test-debian-11-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/5516702708/jobs/10058367344)|
|test-fedora-35-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5516703683/jobs/10058369430)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5516701948/jobs/10058365704)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/5516700801/jobs/10058363438)|
|test-ubuntu-20.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/5516703127/jobs/10058368146)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/5516703510/jobs/10058369013)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5516702187/jobs/10058366116)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/5516702945/jobs/10058367755)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36334:
URL: https://github.com/apache/arrow/pull/36334#issuecomment-1630311531
Revision: 2e3a9eb45bc39fd2628d7132dd9c7a4226c02290
Submitted crossbow builds: [ursacomputing/crossbow @ actions-972a30de43](https://github.com/ursacomputing/crossbow/branches/all?query=actions-972a30de43)
|Task|Status|
|----|------|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5517150898/jobs/10059381324)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/5517149494/jobs/10059378017)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5517148507/jobs/10059375502)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/runs/14936541108)|
|test-cuda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5517151580/jobs/10059382992)|
|test-debian-11-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5517148613/jobs/10059375627)|
|test-debian-11-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/5517151363/jobs/10059382420)|
|test-fedora-35-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5517150274/jobs/10059379795)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5517149762/jobs/10059378553)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/5517150001/jobs/10059379055)|
|test-ubuntu-20.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/5517150563/jobs/10059380572)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/5517150656/jobs/10059380755)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5517149298/jobs/10059377496)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/5517149109/jobs/10059377018)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36334:
URL: https://github.com/apache/arrow/pull/36334#discussion_r1251171656
##########
cpp/src/arrow/util/span.h:
##########
@@ -0,0 +1,129 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+#include <cstring>
+#include <iterator>
+#include <type_traits>
+
+namespace arrow::util {
+
+/// std::span polyfill.
+///
+/// Does not support static extents.
+template <typename T>
+class span {
+ static_assert(sizeof(T),
+ R"(
+std::span allows contiguous_iterators instead of just pointers, the enforcement
+of which requires T to be a complete type. arrow::util::span does not support
+contiguous_iterators, but T is still required to be a complete type to prevent
+writing code which would break when it is replaced by std::span.)");
+
+ public:
+ using element_type = T;
+ using value_type = std::remove_cv_t<T>;
+ using iterator = T*;
+ using const_iterator = T const*;
+
+ span() = default;
+ span(const span&) = default;
+ span& operator=(const span&) = default;
+
+ template <typename M, typename = std::enable_if_t<std::is_same_v<T, M const>>>
+ // NOLINTNEXTLINE runtime/explicit
+ constexpr span(span<M> mut) : span{mut.data(), mut.size()} {}
+
+ constexpr span(T* data, size_t count) : data_{data}, size_{count} {}
+
+ constexpr span(T* begin, T* end)
+ : data_{begin}, size_{static_cast<size_t>(end - begin)} {}
+
+ template <
+ typename R,
+ typename DisableUnlessConstructibleFromDataAndSize =
+ decltype(span<T>(std::data(std::declval<R&>()), std::size(std::declval<R&>()))),
+ 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>>>>
+ // NOLINTNEXTLINE runtime/explicit, non-const reference
+ constexpr span(R& range) : span{std::data(range), std::size(range)} {}
+
+ constexpr T* begin() const { return data_; }
+ constexpr T* end() const { return data_ + size_; }
+ constexpr T* data() const { return data_; }
+
+ constexpr size_t size() const { return size_; }
+ constexpr size_t size_bytes() const { return size_ * sizeof(T); }
+ constexpr bool empty() const { return size_ == 0; }
+
+ constexpr T& operator[](size_t i) { return data_[i]; }
+ constexpr const T& operator[](size_t i) const { return data_[i]; }
+
+ constexpr span subspan(size_t offset) const {
+ if (offset > size_) return {};
Review Comment:
Should we still return a non-null pointer?
```suggestion
if (offset > size_) return {data_, 0};
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36334:
URL: https://github.com/apache/arrow/pull/36334#issuecomment-1629534011
Revision: 9d122c187f74fcd78ea8731fc90265acb9e5d84e
Submitted crossbow builds: [ursacomputing/crossbow @ actions-a551991807](https://github.com/ursacomputing/crossbow/branches/all?query=actions-a551991807)
|Task|Status|
|----|------|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5511866136/jobs/10047972126)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/5511867846/jobs/10047976368)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5511865043/jobs/10047969446)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/runs/14922039855)|
|test-cuda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5511864548/jobs/10047968334)|
|test-debian-11-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5511866593/jobs/10047973273)|
|test-debian-11-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/5511864368/jobs/10047967956)|
|test-fedora-35-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5511867394/jobs/10047975305)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5511866975/jobs/10047974349)|
|test-ubuntu-20.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/5511865846/jobs/10047971415)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/5511866847/jobs/10047973994)|
|test-ubuntu-20.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/5511865546/jobs/10047970592)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/5511864774/jobs/10047968908)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5511865310/jobs/10047970148)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou merged pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #36334:
URL: https://github.com/apache/arrow/pull/36334
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a diff in pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #36334:
URL: https://github.com/apache/arrow/pull/36334#discussion_r1258685074
##########
cpp/src/arrow/util/span.h:
##########
@@ -0,0 +1,129 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+#include <cstring>
+#include <iterator>
+#include <type_traits>
+
+namespace arrow::util {
+
+/// std::span polyfill.
+///
+/// Does not support static extents.
+template <typename T>
+class span {
+ static_assert(sizeof(T),
+ R"(
+std::span allows contiguous_iterators instead of just pointers, the enforcement
+of which requires T to be a complete type. arrow::util::span does not support
+contiguous_iterators, but T is still required to be a complete type to prevent
+writing code which would break when it is replaced by std::span.)");
+
+ public:
+ using element_type = T;
+ using value_type = std::remove_cv_t<T>;
+ using iterator = T*;
+ using const_iterator = T const*;
+
+ span() = default;
+ span(const span&) = default;
+ span& operator=(const span&) = default;
+
+ template <typename M, typename = std::enable_if_t<std::is_same_v<T, M const>>>
+ // NOLINTNEXTLINE runtime/explicit
+ constexpr span(span<M> mut) : span{mut.data(), mut.size()} {}
+
+ constexpr span(T* data, size_t count) : data_{data}, size_{count} {}
+
+ constexpr span(T* begin, T* end)
+ : data_{begin}, size_{static_cast<size_t>(end - begin)} {}
+
+ template <
+ typename R,
+ typename DisableUnlessConstructibleFromDataAndSize =
+ decltype(span<T>(std::data(std::declval<R>()), std::size(std::declval<R>()))),
+ 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>>>>
+ // NOLINTNEXTLINE runtime/explicit, non-const reference
+ constexpr span(R&& range) : span{std::data(range), std::size(range)} {}
+
+ constexpr T* begin() const { return data_; }
+ constexpr T* end() const { return data_ + size_; }
+ constexpr T* data() const { return data_; }
+
+ constexpr size_t size() const { return size_; }
+ constexpr size_t size_bytes() const { return size_ * sizeof(T); }
+ constexpr bool empty() const { return size_ == 0; }
+
+ constexpr T& operator[](size_t i) { return data_[i]; }
+ constexpr const T& operator[](size_t i) const { return data_[i]; }
+
+ constexpr span subspan(size_t offset) const {
+ if (offset > size_) return {data_, data_};
+ return {data_ + offset, size_ - offset};
+ }
+
+ constexpr span subspan(size_t offset, size_t count) const {
+ auto out = subspan(offset);
+ if (count < out.size_) {
+ out.size_ = count;
+ }
+ return out;
+ }
+
+ constexpr bool operator==(span const& other) const {
+ if (size_ != other.size_) return false;
+
+ if constexpr (std::is_integral_v<T>) {
+ return std::memcmp(data_, other.data_, size_bytes()) == 0;
Review Comment:
Added a test for equality
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] conbench-apache-arrow[bot] commented on pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #36334:
URL: https://github.com/apache/arrow/pull/36334#issuecomment-1646093702
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 366dbe1e737fc2fb838180bd1598f5ccd0482310.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/15244386985) has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36334:
URL: https://github.com/apache/arrow/pull/36334#issuecomment-1610198453
@benibus Do you want to review this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a diff in pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #36334:
URL: https://github.com/apache/arrow/pull/36334#discussion_r1254436421
##########
cpp/src/arrow/util/span.h:
##########
@@ -0,0 +1,129 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+#include <cstring>
+#include <iterator>
+#include <type_traits>
+
+namespace arrow::util {
+
+/// std::span polyfill.
+///
+/// Does not support static extents.
+template <typename T>
+class span {
+ static_assert(sizeof(T),
+ R"(
+std::span allows contiguous_iterators instead of just pointers, the enforcement
+of which requires T to be a complete type. arrow::util::span does not support
+contiguous_iterators, but T is still required to be a complete type to prevent
+writing code which would break when it is replaced by std::span.)");
+
+ public:
+ using element_type = T;
+ using value_type = std::remove_cv_t<T>;
+ using iterator = T*;
+ using const_iterator = T const*;
+
+ span() = default;
+ span(const span&) = default;
+ span& operator=(const span&) = default;
+
+ template <typename M, typename = std::enable_if_t<std::is_same_v<T, M const>>>
+ // NOLINTNEXTLINE runtime/explicit
+ constexpr span(span<M> mut) : span{mut.data(), mut.size()} {}
+
+ constexpr span(T* data, size_t count) : data_{data}, size_{count} {}
+
+ constexpr span(T* begin, T* end)
+ : data_{begin}, size_{static_cast<size_t>(end - begin)} {}
+
+ template <
+ typename R,
+ typename DisableUnlessConstructibleFromDataAndSize =
+ decltype(span<T>(std::data(std::declval<R&>()), std::size(std::declval<R&>()))),
+ 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>>>>
+ // NOLINTNEXTLINE runtime/explicit, non-const reference
+ constexpr span(R& range) : span{std::data(range), std::size(range)} {}
+
+ constexpr T* begin() const { return data_; }
+ constexpr T* end() const { return data_ + size_; }
+ constexpr T* data() const { return data_; }
+
+ constexpr size_t size() const { return size_; }
+ constexpr size_t size_bytes() const { return size_ * sizeof(T); }
+ constexpr bool empty() const { return size_ == 0; }
+
+ constexpr T& operator[](size_t i) { return data_[i]; }
+ constexpr const T& operator[](size_t i) const { return data_[i]; }
+
+ constexpr span subspan(size_t offset) const {
+ if (offset > size_) return {};
Review Comment:
alright
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36334:
URL: https://github.com/apache/arrow/pull/36334#discussion_r1258464197
##########
cpp/src/arrow/util/span.h:
##########
@@ -0,0 +1,129 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+#include <cstring>
+#include <iterator>
+#include <type_traits>
+
+namespace arrow::util {
+
+/// std::span polyfill.
+///
+/// Does not support static extents.
+template <typename T>
+class span {
+ static_assert(sizeof(T),
+ R"(
+std::span allows contiguous_iterators instead of just pointers, the enforcement
+of which requires T to be a complete type. arrow::util::span does not support
+contiguous_iterators, but T is still required to be a complete type to prevent
+writing code which would break when it is replaced by std::span.)");
+
+ public:
+ using element_type = T;
+ using value_type = std::remove_cv_t<T>;
+ using iterator = T*;
+ using const_iterator = T const*;
+
+ span() = default;
+ span(const span&) = default;
+ span& operator=(const span&) = default;
+
+ template <typename M, typename = std::enable_if_t<std::is_same_v<T, M const>>>
+ // NOLINTNEXTLINE runtime/explicit
+ constexpr span(span<M> mut) : span{mut.data(), mut.size()} {}
+
+ constexpr span(T* data, size_t count) : data_{data}, size_{count} {}
+
+ constexpr span(T* begin, T* end)
+ : data_{begin}, size_{static_cast<size_t>(end - begin)} {}
+
+ template <
+ typename R,
+ typename DisableUnlessConstructibleFromDataAndSize =
+ decltype(span<T>(std::data(std::declval<R>()), std::size(std::declval<R>()))),
+ 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>>>>
+ // NOLINTNEXTLINE runtime/explicit, non-const reference
+ constexpr span(R&& range) : span{std::data(range), std::size(range)} {}
+
+ constexpr T* begin() const { return data_; }
+ constexpr T* end() const { return data_ + size_; }
+ constexpr T* data() const { return data_; }
+
+ constexpr size_t size() const { return size_; }
+ constexpr size_t size_bytes() const { return size_ * sizeof(T); }
+ constexpr bool empty() const { return size_ == 0; }
+
+ constexpr T& operator[](size_t i) { return data_[i]; }
+ constexpr const T& operator[](size_t i) const { return data_[i]; }
+
+ constexpr span subspan(size_t offset) const {
+ if (offset > size_) return {data_, data_};
+ return {data_ + offset, size_ - offset};
+ }
+
+ constexpr span subspan(size_t offset, size_t count) const {
+ auto out = subspan(offset);
+ if (count < out.size_) {
+ out.size_ = count;
+ }
+ return out;
+ }
+
+ constexpr bool operator==(span const& other) const {
+ if (size_ != other.size_) return false;
+
+ if constexpr (std::is_integral_v<T>) {
+ return std::memcmp(data_, other.data_, size_bytes()) == 0;
Review Comment:
Perhaps because you don't have tests for the equality operator?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on PR #36334:
URL: https://github.com/apache/arrow/pull/36334#issuecomment-1613157620
To workaround https://github.com/apache/arrow/issues/36379 for this PR I could add an overload of FormatMatcherDescription which forwards to the v1.11 one when necessary but won't be called otherwise. However that seems pretty hacky. Please advise
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] benibus commented on a diff in pull request #36334: GH-15281: [C++] Replace bytes_view alias with span
Posted by "benibus (via GitHub)" <gi...@apache.org>.
benibus commented on code in PR #36334:
URL: https://github.com/apache/arrow/pull/36334#discussion_r1247159503
##########
cpp/src/arrow/util/bitmap.h:
##########
@@ -387,10 +384,10 @@ class ARROW_EXPORT Bitmap : public util::ToStringOstreamable<Bitmap>,
int64_t length() const { return length_; }
/// string_view of all bytes which contain any bit in this Bitmap
Review Comment:
Maybe we should alter these method comments now? e.g:
```suggestion
/// span of all bytes which contain any bit in this Bitmap
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org