You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/10/15 17:51:40 UTC
[GitHub] [arrow] bkietz opened a new pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
bkietz opened a new pull request #8472:
URL: https://github.com/apache/arrow/pull/8472
- [ ] Unit tests for `util::Variant<>`
- [ ] Benchmark comparison to ensure compute and nested parquet are not impacted
- [ ] More docstrings and comments
Locally:
```
$ ninja
ninja: no work to do.
$ export OBJS=`ls src/arrow/CMakeFiles/arrow_objlib.dir/compute/**/*.o`
$ git checkout master && rm $OBJS && time ninja $OBJS
real 0m6.137s
user 1m6.760s
sys 0m2.975s
$ git checkout 8113-Implement-a-lighter-weigh && rm $OBJS && time ninja $OBJS
real 0m0.032s
user 0m0.123s
sys 0m0.140s
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-735832288
Rebased, I'm writing some micro-benchmarks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-714625854
This seems to speed up the compute layer a bit.
* Before:
```
ArrayArrayKernel<Add, Int64Type>/32768/100 3781 ns 3780 ns 738685 bytes_per_second=8.07385G/s items_per_second=1083.65M/s null_percent=1 size=32.768k
```
* After:
```
ArrayArrayKernel<Add, Int64Type>/32768/100 3615 ns 3614 ns 777365 bytes_per_second=8.44344G/s items_per_second=1.13326G/s null_percent=1 size=32.768k
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8472: ARROW-8113: [C++] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-737384198
@bkietz You may want to review the changes I've done. I think only docs and comments remain to be done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-724772910
@bkietz Do you plan to update 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#discussion_r505794791
##########
File path: cpp/src/arrow/util/variant.h
##########
@@ -17,17 +17,363 @@
#pragma once
-#include "arrow/vendored/variant.hpp" // IWYU pragma: export
+#include <array>
+#include <cstddef>
+#include <type_traits>
+#include <utility>
+
+#include "arrow/util/macros.h"
namespace arrow {
namespace util {
-using ::mpark::bad_variant_access;
-using ::mpark::get;
-using ::mpark::get_if;
-using ::mpark::holds_alternative;
-using ::mpark::variant;
-using ::mpark::visit;
+/// \brief a std::variant-like discriminated union
+///
+/// Simplifications from std::variant:
+///
+/// - Strictly defaultable. The first type of T... must be nothrow default constructible
+/// and it will be used for default Variants.
+///
+/// - Never valueless_by_exception. std::variant supports a state outside those specified
+/// by T... to which it can return in the event that a constructor throws. If a Variant
+/// would become valueless_by_exception it will instead return to its default state.
+///
+/// - Strictly nothrow move constructible and assignable, which is also required of each
+/// of T...
+///
+/// - Less sophisticated type deduction. std::variant<bool, std::string>("hello") will
+/// intelligently construct std::string while Variant will construct bool.
+///
+/// - Either both copy constructible and assignable or neither (std::variant independently
+/// enables copy construction and copy assignment). Variant is copy constructible if
+/// each of T... is copy constructible and assignable.
+///
+/// - Slimmer interface; several members of std::variant are omitted.
+///
+/// - Throws no exceptions; if a bad_variant_access would be thrown Variant will instead
+/// segfault (nullptr dereference).
+///
+/// - Mutable visit takes a pointer instead of mutable reference or rvalue reference,
+/// which is more conformant with our code style.
+template <typename... T>
+class Variant;
+
+namespace detail {
+
+template <typename T, typename = void>
+struct is_equality_comparable : std::false_type {};
+
+template <typename T>
+struct is_equality_comparable<
+ T, typename std::enable_if<std::is_convertible<
+ decltype(std::declval<T>() == std::declval<T>()), bool>::value>::type>
+ : std::true_type {};
+
+template <bool C, typename T, typename E>
+using conditional_t = typename std::conditional<C, T, E>::type;
+
+template <typename T>
+struct type_constant {
+ using type = T;
+};
+
+template <typename...>
+struct first;
+
+template <typename H, typename... T>
+struct first<H, T...> {
+ using type = H;
+};
+
+template <typename T>
+using decay_t = typename std::decay<T>::type;
+
+template <bool...>
+struct all : std::true_type {};
+
+template <bool H, bool... T>
+struct all<H, T...> : conditional_t<H, all<T...>, std::false_type> {};
+
+template <typename T>
+void copy_construct(void* ptr, const void* other) {
+ new (ptr) T(*static_cast<const T*>(other));
+}
+
+template <typename T>
+void move_construct(void* ptr, void* other) {
+ new (ptr) T(std::move(*static_cast<T*>(other)));
+}
+
+template <typename T>
+void explicit_destroy(void* ptr) {
+ static_cast<T*>(ptr)->~T();
+}
+
+inline void trivial_destroy(void*) {}
+
+template <typename T>
+bool equal(const void* l, const void* r) {
+ return *static_cast<const T*>(l) == *static_cast<const T*>(r);
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<const T&>())) visit_const_ref(
+ Visitor&& visitor, const void* ptr) {
+ return std::forward<Visitor>(visitor)(*static_cast<const T*>(ptr));
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<T*>())) visit_mutable_ptr(
+ Visitor&& visitor, void* ptr) {
+ return std::forward<Visitor>(visitor)(static_cast<T*>(ptr));
+}
+
+template <typename... T>
+struct variant_storage {
+ variant_storage() = default;
+ variant_storage(const variant_storage&) {}
+ variant_storage& operator=(const variant_storage&) { return *this; }
+ variant_storage(variant_storage&&) {}
+ variant_storage& operator=(variant_storage&&) { return *this; }
+ ~variant_storage() {
+ static_assert(offsetof(variant_storage, data_) == 0,
+ "(void*)&variant_storage::data_ == (void*)this");
+ }
+
+ typename std::aligned_union<0, T...>::type data_;
+ uint8_t index_ = 0;
+};
+
+struct delete_copy_constructor {
+ template <typename>
+ struct type {
+ type() = default;
+ type(const type& other) = delete;
+ type& operator=(const type& other) = delete;
+ };
+};
+
+struct explicit_copy_constructor {
+ template <typename Copyable>
+ struct type {
+ type() = default;
+ type(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/false);
+ }
+ type& operator=(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/true);
+ return *this;
+ }
+ };
+};
+
+template <typename V, uint8_t I, typename...>
+struct member_constructor {
+ static void index_of() {}
+};
+
+template <typename V, uint8_t I, typename H, typename... T>
+struct member_constructor<V, I, H, T...> : member_constructor<V, I + 1, T...> {
+ member_constructor() = default;
+
+ using member_constructor<V, I + 1, T...>::member_constructor;
+ using member_constructor<V, I + 1, T...>::operator=;
+ using member_constructor<V, I + 1, T...>::index_of;
+
+ explicit member_constructor(H value) {
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ }
+
+ member_constructor& operator=(H value) {
+ static_cast<V*>(this)->destroy();
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ return *this;
+ }
+
+ static constexpr std::integral_constant<uint8_t, I> index_of(const type_constant<H>&) {
+ return {};
+ }
+};
+
+} // namespace detail
+
+template <typename... T>
+class Variant : detail::variant_storage<T...>,
+ detail::conditional_t<
+ detail::all<(std::is_copy_constructible<T>::value &&
+ std::is_copy_assignable<T>::value)...>::value,
+ detail::explicit_copy_constructor,
+ detail::delete_copy_constructor>::template type<Variant<T...>>,
+ detail::member_constructor<Variant<T...>, 0, T...> {
+ static_assert(detail::all<(std::is_nothrow_move_constructible<T>::value &&
+ std::is_nothrow_move_assignable<T>::value)...>::value,
+ "valueless_by_exception is not supported");
+
+ template <typename U>
+ static constexpr uint8_t index_of() {
+ return detail::member_constructor<Variant<T...>, 0, T...>::index_of(
+ detail::type_constant<U>{});
+ }
+
+ public:
+ using default_type = typename util::detail::first<T...>::type;
+ static_assert(std::is_nothrow_default_constructible<default_type>::value,
+ "valueless_by_exception, non-default constructible are not supported");
+
+ Variant() noexcept { construct_default(); }
+
+ Variant(const Variant& other) = default;
+ Variant& operator=(const Variant& other) = default;
+
+ using detail::member_constructor<Variant<T...>, 0, T...>::member_constructor;
+ using detail::member_constructor<Variant<T...>, 0, T...>::operator=;
+
+ Variant(Variant&& other) noexcept { move(&other, /*assignment=*/false); }
+
+ Variant& operator=(Variant&& other) noexcept {
+ move(&other, /*assignment=*/true);
+ return *this;
+ }
+
+ ~Variant() {
+ static_assert(offsetof(Variant, data_) == 0, "(void*)&Variant::data_ == (void*)this");
+ this->destroy();
+ }
+
+ uint8_t index() const noexcept { return this->index_; }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ const U* get() const noexcept {
+ return index() == I ? reinterpret_cast<const U*>(this) : NULLPTR;
+ }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ U* get() noexcept {
+ return index() == I ? reinterpret_cast<U*>(this) : NULLPTR;
+ }
+
+ template <typename U, typename... A, uint8_t I = index_of<U>()>
+ void emplace(A&&... args) try {
+ this->destroy();
+ new (this) U(std::forward<A>(args)...);
+ this->index_ = I;
+ } catch (...) {
+ construct_default();
+ throw;
+ }
+
+ void swap(Variant& other) noexcept { // NOLINT google-runtime-references
+ Variant tmp = std::move(other);
+ other = std::move(*this);
+ *this = std::move(tmp);
+ }
+
+ private:
+ void move(Variant* other, bool assignment) noexcept {
+ using impl_t = void(void*, void*);
+ static std::array<impl_t*, sizeof...(T)> impl = {detail::move_construct<T>...};
+
+ if (assignment) {
+ this->destroy();
+ }
+
+ auto index = other->index_;
+ impl[index](this, other);
Review comment:
https://godbolt.org/z/jM6Tzx
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8472: ARROW-8113: [C++] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-735866570
@bkietz Do you know why all Github CI checks are skipped here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-709498987
https://issues.apache.org/jira/browse/ARROW-8113
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou closed pull request #8472: ARROW-8113: [C++] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #8472:
URL: https://github.com/apache/arrow/pull/8472
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8472: ARROW-8113: [C++] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-740511215
Ok, I'll rebase a last time to make sure this doesn't break anything.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#discussion_r506422416
##########
File path: cpp/src/arrow/util/variant.h
##########
@@ -17,17 +17,363 @@
#pragma once
-#include "arrow/vendored/variant.hpp" // IWYU pragma: export
+#include <array>
+#include <cstddef>
+#include <type_traits>
+#include <utility>
+
+#include "arrow/util/macros.h"
namespace arrow {
namespace util {
-using ::mpark::bad_variant_access;
-using ::mpark::get;
-using ::mpark::get_if;
-using ::mpark::holds_alternative;
-using ::mpark::variant;
-using ::mpark::visit;
+/// \brief a std::variant-like discriminated union
+///
+/// Simplifications from std::variant:
+///
+/// - Strictly defaultable. The first type of T... must be nothrow default constructible
+/// and it will be used for default Variants.
+///
+/// - Never valueless_by_exception. std::variant supports a state outside those specified
+/// by T... to which it can return in the event that a constructor throws. If a Variant
+/// would become valueless_by_exception it will instead return to its default state.
+///
+/// - Strictly nothrow move constructible and assignable, which is also required of each
+/// of T...
+///
+/// - Less sophisticated type deduction. std::variant<bool, std::string>("hello") will
+/// intelligently construct std::string while Variant will construct bool.
+///
+/// - Either both copy constructible and assignable or neither (std::variant independently
+/// enables copy construction and copy assignment). Variant is copy constructible if
+/// each of T... is copy constructible and assignable.
+///
+/// - Slimmer interface; several members of std::variant are omitted.
+///
+/// - Throws no exceptions; if a bad_variant_access would be thrown Variant will instead
+/// segfault (nullptr dereference).
+///
+/// - Mutable visit takes a pointer instead of mutable reference or rvalue reference,
+/// which is more conformant with our code style.
+template <typename... T>
+class Variant;
+
+namespace detail {
+
+template <typename T, typename = void>
+struct is_equality_comparable : std::false_type {};
+
+template <typename T>
+struct is_equality_comparable<
+ T, typename std::enable_if<std::is_convertible<
+ decltype(std::declval<T>() == std::declval<T>()), bool>::value>::type>
+ : std::true_type {};
+
+template <bool C, typename T, typename E>
+using conditional_t = typename std::conditional<C, T, E>::type;
+
+template <typename T>
+struct type_constant {
+ using type = T;
+};
+
+template <typename...>
+struct first;
+
+template <typename H, typename... T>
+struct first<H, T...> {
+ using type = H;
+};
+
+template <typename T>
+using decay_t = typename std::decay<T>::type;
+
+template <bool...>
+struct all : std::true_type {};
+
+template <bool H, bool... T>
+struct all<H, T...> : conditional_t<H, all<T...>, std::false_type> {};
+
+template <typename T>
+void copy_construct(void* ptr, const void* other) {
+ new (ptr) T(*static_cast<const T*>(other));
+}
+
+template <typename T>
+void move_construct(void* ptr, void* other) {
+ new (ptr) T(std::move(*static_cast<T*>(other)));
+}
+
+template <typename T>
+void explicit_destroy(void* ptr) {
+ static_cast<T*>(ptr)->~T();
+}
+
+inline void trivial_destroy(void*) {}
+
+template <typename T>
+bool equal(const void* l, const void* r) {
+ return *static_cast<const T*>(l) == *static_cast<const T*>(r);
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<const T&>())) visit_const_ref(
+ Visitor&& visitor, const void* ptr) {
+ return std::forward<Visitor>(visitor)(*static_cast<const T*>(ptr));
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<T*>())) visit_mutable_ptr(
+ Visitor&& visitor, void* ptr) {
+ return std::forward<Visitor>(visitor)(static_cast<T*>(ptr));
+}
+
+template <typename... T>
+struct variant_storage {
+ variant_storage() = default;
+ variant_storage(const variant_storage&) {}
+ variant_storage& operator=(const variant_storage&) { return *this; }
+ variant_storage(variant_storage&&) {}
+ variant_storage& operator=(variant_storage&&) { return *this; }
+ ~variant_storage() {
+ static_assert(offsetof(variant_storage, data_) == 0,
+ "(void*)&variant_storage::data_ == (void*)this");
+ }
+
+ typename std::aligned_union<0, T...>::type data_;
+ uint8_t index_ = 0;
+};
+
+struct delete_copy_constructor {
+ template <typename>
+ struct type {
+ type() = default;
+ type(const type& other) = delete;
+ type& operator=(const type& other) = delete;
+ };
+};
+
+struct explicit_copy_constructor {
+ template <typename Copyable>
+ struct type {
+ type() = default;
+ type(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/false);
+ }
+ type& operator=(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/true);
+ return *this;
+ }
+ };
+};
+
+template <typename V, uint8_t I, typename...>
+struct member_constructor {
+ static void index_of() {}
+};
+
+template <typename V, uint8_t I, typename H, typename... T>
+struct member_constructor<V, I, H, T...> : member_constructor<V, I + 1, T...> {
+ member_constructor() = default;
+
+ using member_constructor<V, I + 1, T...>::member_constructor;
+ using member_constructor<V, I + 1, T...>::operator=;
+ using member_constructor<V, I + 1, T...>::index_of;
+
+ explicit member_constructor(H value) {
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ }
+
+ member_constructor& operator=(H value) {
+ static_cast<V*>(this)->destroy();
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ return *this;
+ }
+
+ static constexpr std::integral_constant<uint8_t, I> index_of(const type_constant<H>&) {
+ return {};
+ }
+};
+
+} // namespace detail
+
+template <typename... T>
+class Variant : detail::variant_storage<T...>,
+ detail::conditional_t<
+ detail::all<(std::is_copy_constructible<T>::value &&
+ std::is_copy_assignable<T>::value)...>::value,
+ detail::explicit_copy_constructor,
+ detail::delete_copy_constructor>::template type<Variant<T...>>,
+ detail::member_constructor<Variant<T...>, 0, T...> {
+ static_assert(detail::all<(std::is_nothrow_move_constructible<T>::value &&
+ std::is_nothrow_move_assignable<T>::value)...>::value,
+ "valueless_by_exception is not supported");
+
+ template <typename U>
+ static constexpr uint8_t index_of() {
+ return detail::member_constructor<Variant<T...>, 0, T...>::index_of(
+ detail::type_constant<U>{});
+ }
+
+ public:
+ using default_type = typename util::detail::first<T...>::type;
+ static_assert(std::is_nothrow_default_constructible<default_type>::value,
+ "valueless_by_exception, non-default constructible are not supported");
+
+ Variant() noexcept { construct_default(); }
+
+ Variant(const Variant& other) = default;
+ Variant& operator=(const Variant& other) = default;
+
+ using detail::member_constructor<Variant<T...>, 0, T...>::member_constructor;
+ using detail::member_constructor<Variant<T...>, 0, T...>::operator=;
+
+ Variant(Variant&& other) noexcept { move(&other, /*assignment=*/false); }
+
+ Variant& operator=(Variant&& other) noexcept {
+ move(&other, /*assignment=*/true);
+ return *this;
+ }
+
+ ~Variant() {
+ static_assert(offsetof(Variant, data_) == 0, "(void*)&Variant::data_ == (void*)this");
+ this->destroy();
+ }
+
+ uint8_t index() const noexcept { return this->index_; }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ const U* get() const noexcept {
+ return index() == I ? reinterpret_cast<const U*>(this) : NULLPTR;
+ }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ U* get() noexcept {
+ return index() == I ? reinterpret_cast<U*>(this) : NULLPTR;
+ }
+
+ template <typename U, typename... A, uint8_t I = index_of<U>()>
+ void emplace(A&&... args) try {
+ this->destroy();
+ new (this) U(std::forward<A>(args)...);
+ this->index_ = I;
+ } catch (...) {
+ construct_default();
+ throw;
+ }
+
+ void swap(Variant& other) noexcept { // NOLINT google-runtime-references
+ Variant tmp = std::move(other);
+ other = std::move(*this);
+ *this = std::move(tmp);
+ }
+
+ private:
+ void move(Variant* other, bool assignment) noexcept {
+ using impl_t = void(void*, void*);
+ static std::array<impl_t*, sizeof...(T)> impl = {detail::move_construct<T>...};
+
+ if (assignment) {
+ this->destroy();
+ }
+
+ auto index = other->index_;
+ impl[index](this, other);
Review comment:
Will do, thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8472: ARROW-8113: [C++] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-738106982
I added some docstrings. I think this is ready now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#discussion_r505778397
##########
File path: cpp/src/arrow/util/variant.h
##########
@@ -17,17 +17,363 @@
#pragma once
-#include "arrow/vendored/variant.hpp" // IWYU pragma: export
+#include <array>
+#include <cstddef>
+#include <type_traits>
+#include <utility>
+
+#include "arrow/util/macros.h"
namespace arrow {
namespace util {
-using ::mpark::bad_variant_access;
-using ::mpark::get;
-using ::mpark::get_if;
-using ::mpark::holds_alternative;
-using ::mpark::variant;
-using ::mpark::visit;
+/// \brief a std::variant-like discriminated union
+///
+/// Simplifications from std::variant:
+///
+/// - Strictly defaultable. The first type of T... must be nothrow default constructible
+/// and it will be used for default Variants.
+///
+/// - Never valueless_by_exception. std::variant supports a state outside those specified
+/// by T... to which it can return in the event that a constructor throws. If a Variant
+/// would become valueless_by_exception it will instead return to its default state.
+///
+/// - Strictly nothrow move constructible and assignable, which is also required of each
+/// of T...
+///
+/// - Less sophisticated type deduction. std::variant<bool, std::string>("hello") will
+/// intelligently construct std::string while Variant will construct bool.
+///
+/// - Either both copy constructible and assignable or neither (std::variant independently
+/// enables copy construction and copy assignment). Variant is copy constructible if
+/// each of T... is copy constructible and assignable.
+///
+/// - Slimmer interface; several members of std::variant are omitted.
+///
+/// - Throws no exceptions; if a bad_variant_access would be thrown Variant will instead
+/// segfault (nullptr dereference).
+///
+/// - Mutable visit takes a pointer instead of mutable reference or rvalue reference,
+/// which is more conformant with our code style.
+template <typename... T>
+class Variant;
+
+namespace detail {
+
+template <typename T, typename = void>
+struct is_equality_comparable : std::false_type {};
+
+template <typename T>
+struct is_equality_comparable<
+ T, typename std::enable_if<std::is_convertible<
+ decltype(std::declval<T>() == std::declval<T>()), bool>::value>::type>
+ : std::true_type {};
+
+template <bool C, typename T, typename E>
+using conditional_t = typename std::conditional<C, T, E>::type;
+
+template <typename T>
+struct type_constant {
+ using type = T;
+};
+
+template <typename...>
+struct first;
+
+template <typename H, typename... T>
+struct first<H, T...> {
+ using type = H;
+};
+
+template <typename T>
+using decay_t = typename std::decay<T>::type;
+
+template <bool...>
+struct all : std::true_type {};
+
+template <bool H, bool... T>
+struct all<H, T...> : conditional_t<H, all<T...>, std::false_type> {};
+
+template <typename T>
+void copy_construct(void* ptr, const void* other) {
+ new (ptr) T(*static_cast<const T*>(other));
+}
+
+template <typename T>
+void move_construct(void* ptr, void* other) {
+ new (ptr) T(std::move(*static_cast<T*>(other)));
+}
+
+template <typename T>
+void explicit_destroy(void* ptr) {
+ static_cast<T*>(ptr)->~T();
+}
+
+inline void trivial_destroy(void*) {}
+
+template <typename T>
+bool equal(const void* l, const void* r) {
+ return *static_cast<const T*>(l) == *static_cast<const T*>(r);
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<const T&>())) visit_const_ref(
+ Visitor&& visitor, const void* ptr) {
+ return std::forward<Visitor>(visitor)(*static_cast<const T*>(ptr));
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<T*>())) visit_mutable_ptr(
+ Visitor&& visitor, void* ptr) {
+ return std::forward<Visitor>(visitor)(static_cast<T*>(ptr));
+}
+
+template <typename... T>
+struct variant_storage {
+ variant_storage() = default;
+ variant_storage(const variant_storage&) {}
+ variant_storage& operator=(const variant_storage&) { return *this; }
+ variant_storage(variant_storage&&) {}
+ variant_storage& operator=(variant_storage&&) { return *this; }
+ ~variant_storage() {
+ static_assert(offsetof(variant_storage, data_) == 0,
+ "(void*)&variant_storage::data_ == (void*)this");
+ }
+
+ typename std::aligned_union<0, T...>::type data_;
+ uint8_t index_ = 0;
+};
+
+struct delete_copy_constructor {
+ template <typename>
+ struct type {
+ type() = default;
+ type(const type& other) = delete;
+ type& operator=(const type& other) = delete;
+ };
+};
+
+struct explicit_copy_constructor {
+ template <typename Copyable>
+ struct type {
+ type() = default;
+ type(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/false);
+ }
+ type& operator=(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/true);
+ return *this;
+ }
+ };
+};
+
+template <typename V, uint8_t I, typename...>
+struct member_constructor {
+ static void index_of() {}
+};
+
+template <typename V, uint8_t I, typename H, typename... T>
+struct member_constructor<V, I, H, T...> : member_constructor<V, I + 1, T...> {
+ member_constructor() = default;
+
+ using member_constructor<V, I + 1, T...>::member_constructor;
+ using member_constructor<V, I + 1, T...>::operator=;
+ using member_constructor<V, I + 1, T...>::index_of;
+
+ explicit member_constructor(H value) {
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ }
+
+ member_constructor& operator=(H value) {
+ static_cast<V*>(this)->destroy();
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ return *this;
+ }
+
+ static constexpr std::integral_constant<uint8_t, I> index_of(const type_constant<H>&) {
+ return {};
+ }
+};
+
+} // namespace detail
+
+template <typename... T>
+class Variant : detail::variant_storage<T...>,
+ detail::conditional_t<
+ detail::all<(std::is_copy_constructible<T>::value &&
+ std::is_copy_assignable<T>::value)...>::value,
+ detail::explicit_copy_constructor,
+ detail::delete_copy_constructor>::template type<Variant<T...>>,
+ detail::member_constructor<Variant<T...>, 0, T...> {
+ static_assert(detail::all<(std::is_nothrow_move_constructible<T>::value &&
+ std::is_nothrow_move_assignable<T>::value)...>::value,
+ "valueless_by_exception is not supported");
+
+ template <typename U>
+ static constexpr uint8_t index_of() {
+ return detail::member_constructor<Variant<T...>, 0, T...>::index_of(
+ detail::type_constant<U>{});
+ }
+
+ public:
+ using default_type = typename util::detail::first<T...>::type;
+ static_assert(std::is_nothrow_default_constructible<default_type>::value,
+ "valueless_by_exception, non-default constructible are not supported");
+
+ Variant() noexcept { construct_default(); }
+
+ Variant(const Variant& other) = default;
+ Variant& operator=(const Variant& other) = default;
+
+ using detail::member_constructor<Variant<T...>, 0, T...>::member_constructor;
+ using detail::member_constructor<Variant<T...>, 0, T...>::operator=;
+
+ Variant(Variant&& other) noexcept { move(&other, /*assignment=*/false); }
+
+ Variant& operator=(Variant&& other) noexcept {
+ move(&other, /*assignment=*/true);
+ return *this;
+ }
+
+ ~Variant() {
+ static_assert(offsetof(Variant, data_) == 0, "(void*)&Variant::data_ == (void*)this");
+ this->destroy();
+ }
+
+ uint8_t index() const noexcept { return this->index_; }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ const U* get() const noexcept {
+ return index() == I ? reinterpret_cast<const U*>(this) : NULLPTR;
+ }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ U* get() noexcept {
+ return index() == I ? reinterpret_cast<U*>(this) : NULLPTR;
+ }
+
+ template <typename U, typename... A, uint8_t I = index_of<U>()>
+ void emplace(A&&... args) try {
+ this->destroy();
+ new (this) U(std::forward<A>(args)...);
+ this->index_ = I;
+ } catch (...) {
+ construct_default();
+ throw;
+ }
+
+ void swap(Variant& other) noexcept { // NOLINT google-runtime-references
+ Variant tmp = std::move(other);
+ other = std::move(*this);
+ *this = std::move(tmp);
+ }
+
+ private:
+ void move(Variant* other, bool assignment) noexcept {
+ using impl_t = void(void*, void*);
+ static std::array<impl_t*, sizeof...(T)> impl = {detail::move_construct<T>...};
+
+ if (assignment) {
+ this->destroy();
+ }
+
+ auto index = other->index_;
+ impl[index](this, other);
Review comment:
You could do something like:
```c++
template <typename V, uint8_t I, typename H, typename... T>
struct member_constructor<V, I, H, T...> : member_constructor<V, I + 1, T...> {
// ...
void move_to(uint8_t index, Variant* target) {
if (index == I) {
detail::move_construct(target, this);
} else {
member_constructor<V, I + 1, T...>::move_to(index, target);
}
}
```
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-712563172
@bkietz presumably this yields smaller code size, too?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#discussion_r505774708
##########
File path: cpp/src/arrow/util/variant.h
##########
@@ -17,17 +17,363 @@
#pragma once
-#include "arrow/vendored/variant.hpp" // IWYU pragma: export
+#include <array>
+#include <cstddef>
+#include <type_traits>
+#include <utility>
+
+#include "arrow/util/macros.h"
namespace arrow {
namespace util {
-using ::mpark::bad_variant_access;
-using ::mpark::get;
-using ::mpark::get_if;
-using ::mpark::holds_alternative;
-using ::mpark::variant;
-using ::mpark::visit;
+/// \brief a std::variant-like discriminated union
+///
+/// Simplifications from std::variant:
+///
+/// - Strictly defaultable. The first type of T... must be nothrow default constructible
+/// and it will be used for default Variants.
+///
+/// - Never valueless_by_exception. std::variant supports a state outside those specified
+/// by T... to which it can return in the event that a constructor throws. If a Variant
+/// would become valueless_by_exception it will instead return to its default state.
+///
+/// - Strictly nothrow move constructible and assignable, which is also required of each
+/// of T...
+///
+/// - Less sophisticated type deduction. std::variant<bool, std::string>("hello") will
+/// intelligently construct std::string while Variant will construct bool.
+///
+/// - Either both copy constructible and assignable or neither (std::variant independently
+/// enables copy construction and copy assignment). Variant is copy constructible if
+/// each of T... is copy constructible and assignable.
+///
+/// - Slimmer interface; several members of std::variant are omitted.
+///
+/// - Throws no exceptions; if a bad_variant_access would be thrown Variant will instead
+/// segfault (nullptr dereference).
+///
+/// - Mutable visit takes a pointer instead of mutable reference or rvalue reference,
+/// which is more conformant with our code style.
+template <typename... T>
+class Variant;
+
+namespace detail {
+
+template <typename T, typename = void>
+struct is_equality_comparable : std::false_type {};
+
+template <typename T>
+struct is_equality_comparable<
+ T, typename std::enable_if<std::is_convertible<
+ decltype(std::declval<T>() == std::declval<T>()), bool>::value>::type>
+ : std::true_type {};
+
+template <bool C, typename T, typename E>
+using conditional_t = typename std::conditional<C, T, E>::type;
+
+template <typename T>
+struct type_constant {
+ using type = T;
+};
+
+template <typename...>
+struct first;
+
+template <typename H, typename... T>
+struct first<H, T...> {
+ using type = H;
+};
+
+template <typename T>
+using decay_t = typename std::decay<T>::type;
+
+template <bool...>
+struct all : std::true_type {};
+
+template <bool H, bool... T>
+struct all<H, T...> : conditional_t<H, all<T...>, std::false_type> {};
+
+template <typename T>
+void copy_construct(void* ptr, const void* other) {
+ new (ptr) T(*static_cast<const T*>(other));
+}
+
+template <typename T>
+void move_construct(void* ptr, void* other) {
+ new (ptr) T(std::move(*static_cast<T*>(other)));
+}
+
+template <typename T>
+void explicit_destroy(void* ptr) {
+ static_cast<T*>(ptr)->~T();
+}
+
+inline void trivial_destroy(void*) {}
+
+template <typename T>
+bool equal(const void* l, const void* r) {
+ return *static_cast<const T*>(l) == *static_cast<const T*>(r);
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<const T&>())) visit_const_ref(
+ Visitor&& visitor, const void* ptr) {
+ return std::forward<Visitor>(visitor)(*static_cast<const T*>(ptr));
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<T*>())) visit_mutable_ptr(
+ Visitor&& visitor, void* ptr) {
+ return std::forward<Visitor>(visitor)(static_cast<T*>(ptr));
+}
+
+template <typename... T>
+struct variant_storage {
+ variant_storage() = default;
+ variant_storage(const variant_storage&) {}
+ variant_storage& operator=(const variant_storage&) { return *this; }
+ variant_storage(variant_storage&&) {}
+ variant_storage& operator=(variant_storage&&) { return *this; }
+ ~variant_storage() {
+ static_assert(offsetof(variant_storage, data_) == 0,
+ "(void*)&variant_storage::data_ == (void*)this");
+ }
+
+ typename std::aligned_union<0, T...>::type data_;
+ uint8_t index_ = 0;
+};
+
+struct delete_copy_constructor {
+ template <typename>
+ struct type {
+ type() = default;
+ type(const type& other) = delete;
+ type& operator=(const type& other) = delete;
+ };
+};
+
+struct explicit_copy_constructor {
+ template <typename Copyable>
+ struct type {
+ type() = default;
+ type(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/false);
+ }
+ type& operator=(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/true);
+ return *this;
+ }
+ };
+};
+
+template <typename V, uint8_t I, typename...>
+struct member_constructor {
+ static void index_of() {}
+};
+
+template <typename V, uint8_t I, typename H, typename... T>
+struct member_constructor<V, I, H, T...> : member_constructor<V, I + 1, T...> {
+ member_constructor() = default;
+
+ using member_constructor<V, I + 1, T...>::member_constructor;
+ using member_constructor<V, I + 1, T...>::operator=;
+ using member_constructor<V, I + 1, T...>::index_of;
+
+ explicit member_constructor(H value) {
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ }
+
+ member_constructor& operator=(H value) {
+ static_cast<V*>(this)->destroy();
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ return *this;
+ }
+
+ static constexpr std::integral_constant<uint8_t, I> index_of(const type_constant<H>&) {
+ return {};
+ }
+};
+
+} // namespace detail
+
+template <typename... T>
+class Variant : detail::variant_storage<T...>,
+ detail::conditional_t<
+ detail::all<(std::is_copy_constructible<T>::value &&
+ std::is_copy_assignable<T>::value)...>::value,
+ detail::explicit_copy_constructor,
+ detail::delete_copy_constructor>::template type<Variant<T...>>,
+ detail::member_constructor<Variant<T...>, 0, T...> {
+ static_assert(detail::all<(std::is_nothrow_move_constructible<T>::value &&
+ std::is_nothrow_move_assignable<T>::value)...>::value,
+ "valueless_by_exception is not supported");
+
+ template <typename U>
+ static constexpr uint8_t index_of() {
+ return detail::member_constructor<Variant<T...>, 0, T...>::index_of(
+ detail::type_constant<U>{});
+ }
+
+ public:
+ using default_type = typename util::detail::first<T...>::type;
+ static_assert(std::is_nothrow_default_constructible<default_type>::value,
+ "valueless_by_exception, non-default constructible are not supported");
+
+ Variant() noexcept { construct_default(); }
+
+ Variant(const Variant& other) = default;
+ Variant& operator=(const Variant& other) = default;
+
+ using detail::member_constructor<Variant<T...>, 0, T...>::member_constructor;
+ using detail::member_constructor<Variant<T...>, 0, T...>::operator=;
+
+ Variant(Variant&& other) noexcept { move(&other, /*assignment=*/false); }
+
+ Variant& operator=(Variant&& other) noexcept {
+ move(&other, /*assignment=*/true);
+ return *this;
+ }
+
+ ~Variant() {
+ static_assert(offsetof(Variant, data_) == 0, "(void*)&Variant::data_ == (void*)this");
+ this->destroy();
+ }
+
+ uint8_t index() const noexcept { return this->index_; }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ const U* get() const noexcept {
+ return index() == I ? reinterpret_cast<const U*>(this) : NULLPTR;
+ }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ U* get() noexcept {
+ return index() == I ? reinterpret_cast<U*>(this) : NULLPTR;
+ }
+
+ template <typename U, typename... A, uint8_t I = index_of<U>()>
+ void emplace(A&&... args) try {
+ this->destroy();
+ new (this) U(std::forward<A>(args)...);
+ this->index_ = I;
+ } catch (...) {
+ construct_default();
+ throw;
+ }
+
+ void swap(Variant& other) noexcept { // NOLINT google-runtime-references
+ Variant tmp = std::move(other);
+ other = std::move(*this);
+ *this = std::move(tmp);
+ }
+
+ private:
+ void move(Variant* other, bool assignment) noexcept {
+ using impl_t = void(void*, void*);
+ static std::array<impl_t*, sizeof...(T)> impl = {detail::move_construct<T>...};
+
+ if (assignment) {
+ this->destroy();
+ }
+
+ auto index = other->index_;
+ impl[index](this, other);
Review comment:
Clang 10 succeeds: https://godbolt.org/z/ab8Gaf
Moreover, function tables are also the approach taken by vendored/variant.hpp for c++11 when last I checked
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#discussion_r505766872
##########
File path: cpp/src/arrow/util/variant.h
##########
@@ -17,17 +17,363 @@
#pragma once
-#include "arrow/vendored/variant.hpp" // IWYU pragma: export
+#include <array>
+#include <cstddef>
+#include <type_traits>
+#include <utility>
+
+#include "arrow/util/macros.h"
namespace arrow {
namespace util {
-using ::mpark::bad_variant_access;
-using ::mpark::get;
-using ::mpark::get_if;
-using ::mpark::holds_alternative;
-using ::mpark::variant;
-using ::mpark::visit;
+/// \brief a std::variant-like discriminated union
+///
+/// Simplifications from std::variant:
+///
+/// - Strictly defaultable. The first type of T... must be nothrow default constructible
+/// and it will be used for default Variants.
+///
+/// - Never valueless_by_exception. std::variant supports a state outside those specified
+/// by T... to which it can return in the event that a constructor throws. If a Variant
+/// would become valueless_by_exception it will instead return to its default state.
+///
+/// - Strictly nothrow move constructible and assignable, which is also required of each
+/// of T...
+///
+/// - Less sophisticated type deduction. std::variant<bool, std::string>("hello") will
+/// intelligently construct std::string while Variant will construct bool.
+///
+/// - Either both copy constructible and assignable or neither (std::variant independently
+/// enables copy construction and copy assignment). Variant is copy constructible if
+/// each of T... is copy constructible and assignable.
+///
+/// - Slimmer interface; several members of std::variant are omitted.
+///
+/// - Throws no exceptions; if a bad_variant_access would be thrown Variant will instead
+/// segfault (nullptr dereference).
+///
+/// - Mutable visit takes a pointer instead of mutable reference or rvalue reference,
+/// which is more conformant with our code style.
+template <typename... T>
+class Variant;
+
+namespace detail {
+
+template <typename T, typename = void>
+struct is_equality_comparable : std::false_type {};
+
+template <typename T>
+struct is_equality_comparable<
+ T, typename std::enable_if<std::is_convertible<
+ decltype(std::declval<T>() == std::declval<T>()), bool>::value>::type>
+ : std::true_type {};
+
+template <bool C, typename T, typename E>
+using conditional_t = typename std::conditional<C, T, E>::type;
+
+template <typename T>
+struct type_constant {
+ using type = T;
+};
+
+template <typename...>
+struct first;
+
+template <typename H, typename... T>
+struct first<H, T...> {
+ using type = H;
+};
+
+template <typename T>
+using decay_t = typename std::decay<T>::type;
+
+template <bool...>
+struct all : std::true_type {};
+
+template <bool H, bool... T>
+struct all<H, T...> : conditional_t<H, all<T...>, std::false_type> {};
+
+template <typename T>
+void copy_construct(void* ptr, const void* other) {
+ new (ptr) T(*static_cast<const T*>(other));
+}
+
+template <typename T>
+void move_construct(void* ptr, void* other) {
+ new (ptr) T(std::move(*static_cast<T*>(other)));
+}
+
+template <typename T>
+void explicit_destroy(void* ptr) {
+ static_cast<T*>(ptr)->~T();
+}
+
+inline void trivial_destroy(void*) {}
+
+template <typename T>
+bool equal(const void* l, const void* r) {
+ return *static_cast<const T*>(l) == *static_cast<const T*>(r);
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<const T&>())) visit_const_ref(
+ Visitor&& visitor, const void* ptr) {
+ return std::forward<Visitor>(visitor)(*static_cast<const T*>(ptr));
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<T*>())) visit_mutable_ptr(
+ Visitor&& visitor, void* ptr) {
+ return std::forward<Visitor>(visitor)(static_cast<T*>(ptr));
+}
+
+template <typename... T>
+struct variant_storage {
+ variant_storage() = default;
+ variant_storage(const variant_storage&) {}
+ variant_storage& operator=(const variant_storage&) { return *this; }
+ variant_storage(variant_storage&&) {}
+ variant_storage& operator=(variant_storage&&) { return *this; }
+ ~variant_storage() {
+ static_assert(offsetof(variant_storage, data_) == 0,
+ "(void*)&variant_storage::data_ == (void*)this");
+ }
+
+ typename std::aligned_union<0, T...>::type data_;
+ uint8_t index_ = 0;
+};
+
+struct delete_copy_constructor {
+ template <typename>
+ struct type {
+ type() = default;
+ type(const type& other) = delete;
+ type& operator=(const type& other) = delete;
+ };
+};
+
+struct explicit_copy_constructor {
+ template <typename Copyable>
+ struct type {
+ type() = default;
+ type(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/false);
+ }
+ type& operator=(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/true);
+ return *this;
+ }
+ };
+};
+
+template <typename V, uint8_t I, typename...>
+struct member_constructor {
+ static void index_of() {}
+};
+
+template <typename V, uint8_t I, typename H, typename... T>
+struct member_constructor<V, I, H, T...> : member_constructor<V, I + 1, T...> {
+ member_constructor() = default;
+
+ using member_constructor<V, I + 1, T...>::member_constructor;
+ using member_constructor<V, I + 1, T...>::operator=;
+ using member_constructor<V, I + 1, T...>::index_of;
+
+ explicit member_constructor(H value) {
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ }
+
+ member_constructor& operator=(H value) {
+ static_cast<V*>(this)->destroy();
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ return *this;
+ }
+
+ static constexpr std::integral_constant<uint8_t, I> index_of(const type_constant<H>&) {
+ return {};
+ }
+};
+
+} // namespace detail
+
+template <typename... T>
+class Variant : detail::variant_storage<T...>,
+ detail::conditional_t<
+ detail::all<(std::is_copy_constructible<T>::value &&
+ std::is_copy_assignable<T>::value)...>::value,
+ detail::explicit_copy_constructor,
+ detail::delete_copy_constructor>::template type<Variant<T...>>,
+ detail::member_constructor<Variant<T...>, 0, T...> {
+ static_assert(detail::all<(std::is_nothrow_move_constructible<T>::value &&
+ std::is_nothrow_move_assignable<T>::value)...>::value,
+ "valueless_by_exception is not supported");
+
+ template <typename U>
+ static constexpr uint8_t index_of() {
+ return detail::member_constructor<Variant<T...>, 0, T...>::index_of(
+ detail::type_constant<U>{});
+ }
+
+ public:
+ using default_type = typename util::detail::first<T...>::type;
+ static_assert(std::is_nothrow_default_constructible<default_type>::value,
+ "valueless_by_exception, non-default constructible are not supported");
+
+ Variant() noexcept { construct_default(); }
+
+ Variant(const Variant& other) = default;
+ Variant& operator=(const Variant& other) = default;
+
+ using detail::member_constructor<Variant<T...>, 0, T...>::member_constructor;
+ using detail::member_constructor<Variant<T...>, 0, T...>::operator=;
+
+ Variant(Variant&& other) noexcept { move(&other, /*assignment=*/false); }
+
+ Variant& operator=(Variant&& other) noexcept {
+ move(&other, /*assignment=*/true);
+ return *this;
+ }
+
+ ~Variant() {
+ static_assert(offsetof(Variant, data_) == 0, "(void*)&Variant::data_ == (void*)this");
+ this->destroy();
+ }
+
+ uint8_t index() const noexcept { return this->index_; }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ const U* get() const noexcept {
+ return index() == I ? reinterpret_cast<const U*>(this) : NULLPTR;
+ }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ U* get() noexcept {
+ return index() == I ? reinterpret_cast<U*>(this) : NULLPTR;
+ }
+
+ template <typename U, typename... A, uint8_t I = index_of<U>()>
+ void emplace(A&&... args) try {
+ this->destroy();
+ new (this) U(std::forward<A>(args)...);
+ this->index_ = I;
+ } catch (...) {
+ construct_default();
+ throw;
+ }
+
+ void swap(Variant& other) noexcept { // NOLINT google-runtime-references
+ Variant tmp = std::move(other);
+ other = std::move(*this);
+ *this = std::move(tmp);
+ }
+
+ private:
+ void move(Variant* other, bool assignment) noexcept {
+ using impl_t = void(void*, void*);
+ static std::array<impl_t*, sizeof...(T)> impl = {detail::move_construct<T>...};
Review comment:
Whoops, correct
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-726799854
@pitrou yes but not soon
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#discussion_r505776377
##########
File path: cpp/src/arrow/util/variant.h
##########
@@ -17,17 +17,363 @@
#pragma once
-#include "arrow/vendored/variant.hpp" // IWYU pragma: export
+#include <array>
+#include <cstddef>
+#include <type_traits>
+#include <utility>
+
+#include "arrow/util/macros.h"
namespace arrow {
namespace util {
-using ::mpark::bad_variant_access;
-using ::mpark::get;
-using ::mpark::get_if;
-using ::mpark::holds_alternative;
-using ::mpark::variant;
-using ::mpark::visit;
+/// \brief a std::variant-like discriminated union
+///
+/// Simplifications from std::variant:
+///
+/// - Strictly defaultable. The first type of T... must be nothrow default constructible
+/// and it will be used for default Variants.
+///
+/// - Never valueless_by_exception. std::variant supports a state outside those specified
+/// by T... to which it can return in the event that a constructor throws. If a Variant
+/// would become valueless_by_exception it will instead return to its default state.
+///
+/// - Strictly nothrow move constructible and assignable, which is also required of each
+/// of T...
+///
+/// - Less sophisticated type deduction. std::variant<bool, std::string>("hello") will
+/// intelligently construct std::string while Variant will construct bool.
+///
+/// - Either both copy constructible and assignable or neither (std::variant independently
+/// enables copy construction and copy assignment). Variant is copy constructible if
+/// each of T... is copy constructible and assignable.
+///
+/// - Slimmer interface; several members of std::variant are omitted.
+///
+/// - Throws no exceptions; if a bad_variant_access would be thrown Variant will instead
+/// segfault (nullptr dereference).
+///
+/// - Mutable visit takes a pointer instead of mutable reference or rvalue reference,
+/// which is more conformant with our code style.
+template <typename... T>
+class Variant;
+
+namespace detail {
+
+template <typename T, typename = void>
+struct is_equality_comparable : std::false_type {};
+
+template <typename T>
+struct is_equality_comparable<
+ T, typename std::enable_if<std::is_convertible<
+ decltype(std::declval<T>() == std::declval<T>()), bool>::value>::type>
+ : std::true_type {};
+
+template <bool C, typename T, typename E>
+using conditional_t = typename std::conditional<C, T, E>::type;
+
+template <typename T>
+struct type_constant {
+ using type = T;
+};
+
+template <typename...>
+struct first;
+
+template <typename H, typename... T>
+struct first<H, T...> {
+ using type = H;
+};
+
+template <typename T>
+using decay_t = typename std::decay<T>::type;
+
+template <bool...>
+struct all : std::true_type {};
+
+template <bool H, bool... T>
+struct all<H, T...> : conditional_t<H, all<T...>, std::false_type> {};
+
+template <typename T>
+void copy_construct(void* ptr, const void* other) {
+ new (ptr) T(*static_cast<const T*>(other));
+}
+
+template <typename T>
+void move_construct(void* ptr, void* other) {
+ new (ptr) T(std::move(*static_cast<T*>(other)));
+}
+
+template <typename T>
+void explicit_destroy(void* ptr) {
+ static_cast<T*>(ptr)->~T();
+}
+
+inline void trivial_destroy(void*) {}
+
+template <typename T>
+bool equal(const void* l, const void* r) {
+ return *static_cast<const T*>(l) == *static_cast<const T*>(r);
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<const T&>())) visit_const_ref(
+ Visitor&& visitor, const void* ptr) {
+ return std::forward<Visitor>(visitor)(*static_cast<const T*>(ptr));
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<T*>())) visit_mutable_ptr(
+ Visitor&& visitor, void* ptr) {
+ return std::forward<Visitor>(visitor)(static_cast<T*>(ptr));
+}
+
+template <typename... T>
+struct variant_storage {
+ variant_storage() = default;
+ variant_storage(const variant_storage&) {}
+ variant_storage& operator=(const variant_storage&) { return *this; }
+ variant_storage(variant_storage&&) {}
+ variant_storage& operator=(variant_storage&&) { return *this; }
+ ~variant_storage() {
+ static_assert(offsetof(variant_storage, data_) == 0,
+ "(void*)&variant_storage::data_ == (void*)this");
+ }
+
+ typename std::aligned_union<0, T...>::type data_;
+ uint8_t index_ = 0;
+};
+
+struct delete_copy_constructor {
+ template <typename>
+ struct type {
+ type() = default;
+ type(const type& other) = delete;
+ type& operator=(const type& other) = delete;
+ };
+};
+
+struct explicit_copy_constructor {
+ template <typename Copyable>
+ struct type {
+ type() = default;
+ type(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/false);
+ }
+ type& operator=(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/true);
+ return *this;
+ }
+ };
+};
+
+template <typename V, uint8_t I, typename...>
+struct member_constructor {
+ static void index_of() {}
+};
+
+template <typename V, uint8_t I, typename H, typename... T>
+struct member_constructor<V, I, H, T...> : member_constructor<V, I + 1, T...> {
+ member_constructor() = default;
+
+ using member_constructor<V, I + 1, T...>::member_constructor;
+ using member_constructor<V, I + 1, T...>::operator=;
+ using member_constructor<V, I + 1, T...>::index_of;
+
+ explicit member_constructor(H value) {
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ }
+
+ member_constructor& operator=(H value) {
+ static_cast<V*>(this)->destroy();
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ return *this;
+ }
+
+ static constexpr std::integral_constant<uint8_t, I> index_of(const type_constant<H>&) {
+ return {};
+ }
+};
+
+} // namespace detail
+
+template <typename... T>
+class Variant : detail::variant_storage<T...>,
+ detail::conditional_t<
+ detail::all<(std::is_copy_constructible<T>::value &&
+ std::is_copy_assignable<T>::value)...>::value,
+ detail::explicit_copy_constructor,
+ detail::delete_copy_constructor>::template type<Variant<T...>>,
+ detail::member_constructor<Variant<T...>, 0, T...> {
+ static_assert(detail::all<(std::is_nothrow_move_constructible<T>::value &&
+ std::is_nothrow_move_assignable<T>::value)...>::value,
+ "valueless_by_exception is not supported");
+
+ template <typename U>
+ static constexpr uint8_t index_of() {
+ return detail::member_constructor<Variant<T...>, 0, T...>::index_of(
+ detail::type_constant<U>{});
+ }
+
+ public:
+ using default_type = typename util::detail::first<T...>::type;
+ static_assert(std::is_nothrow_default_constructible<default_type>::value,
+ "valueless_by_exception, non-default constructible are not supported");
+
+ Variant() noexcept { construct_default(); }
+
+ Variant(const Variant& other) = default;
+ Variant& operator=(const Variant& other) = default;
+
+ using detail::member_constructor<Variant<T...>, 0, T...>::member_constructor;
+ using detail::member_constructor<Variant<T...>, 0, T...>::operator=;
+
+ Variant(Variant&& other) noexcept { move(&other, /*assignment=*/false); }
+
+ Variant& operator=(Variant&& other) noexcept {
+ move(&other, /*assignment=*/true);
+ return *this;
+ }
+
+ ~Variant() {
+ static_assert(offsetof(Variant, data_) == 0, "(void*)&Variant::data_ == (void*)this");
+ this->destroy();
+ }
+
+ uint8_t index() const noexcept { return this->index_; }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ const U* get() const noexcept {
+ return index() == I ? reinterpret_cast<const U*>(this) : NULLPTR;
+ }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ U* get() noexcept {
+ return index() == I ? reinterpret_cast<U*>(this) : NULLPTR;
+ }
+
+ template <typename U, typename... A, uint8_t I = index_of<U>()>
+ void emplace(A&&... args) try {
+ this->destroy();
+ new (this) U(std::forward<A>(args)...);
+ this->index_ = I;
+ } catch (...) {
+ construct_default();
+ throw;
+ }
+
+ void swap(Variant& other) noexcept { // NOLINT google-runtime-references
+ Variant tmp = std::move(other);
+ other = std::move(*this);
+ *this = std::move(tmp);
+ }
+
+ private:
+ void move(Variant* other, bool assignment) noexcept {
+ using impl_t = void(void*, void*);
+ static std::array<impl_t*, sizeof...(T)> impl = {detail::move_construct<T>...};
+
+ if (assignment) {
+ this->destroy();
+ }
+
+ auto index = other->index_;
+ impl[index](this, other);
Review comment:
I wouldn't call that succeed. `print_using_table` uses an indirect jump while `print_using_switch` does inline comparisons.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8472: ARROW-8113: [C++] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-740115774
@pitrou thanks for picking this up! Looks good to me
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8472: ARROW-8113: [C++] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-735861424
Compute overhead benchmarks:
* before:
```
BM_CastDispatch 971704 ns 971533 ns 698 items_per_second=1054k/s
BM_CastDispatchBaseline 97365 ns 97352 ns 6955 items_per_second=10.5185M/s
BM_AddDispatch 165 ns 165 ns 4232825 items_per_second=6.07671M/s
BM_ExecuteScalarFunctionOnScalar 4666801 ns 4665955 ns 149 items_per_second=2.14318M/s
BM_ExecuteScalarKernelOnScalar 809563 ns 809419 ns 849 items_per_second=12.3545M/s
```
* after:
```
BM_CastDispatch 914006 ns 913869 ns 759 items_per_second=1.12051M/s
BM_CastDispatchBaseline 95985 ns 95973 ns 7287 items_per_second=10.6697M/s
BM_AddDispatch 167 ns 167 ns 4137185 items_per_second=5.99082M/s
BM_ExecuteScalarFunctionOnScalar 4345926 ns 4345337 ns 161 items_per_second=2.30132M/s
BM_ExecuteScalarKernelOnScalar 847525 ns 847411 ns 835 items_per_second=11.8007M/s
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8472: ARROW-8113: [C++] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-738128198
Hmm, I forgot to push some changes...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#discussion_r505778397
##########
File path: cpp/src/arrow/util/variant.h
##########
@@ -17,17 +17,363 @@
#pragma once
-#include "arrow/vendored/variant.hpp" // IWYU pragma: export
+#include <array>
+#include <cstddef>
+#include <type_traits>
+#include <utility>
+
+#include "arrow/util/macros.h"
namespace arrow {
namespace util {
-using ::mpark::bad_variant_access;
-using ::mpark::get;
-using ::mpark::get_if;
-using ::mpark::holds_alternative;
-using ::mpark::variant;
-using ::mpark::visit;
+/// \brief a std::variant-like discriminated union
+///
+/// Simplifications from std::variant:
+///
+/// - Strictly defaultable. The first type of T... must be nothrow default constructible
+/// and it will be used for default Variants.
+///
+/// - Never valueless_by_exception. std::variant supports a state outside those specified
+/// by T... to which it can return in the event that a constructor throws. If a Variant
+/// would become valueless_by_exception it will instead return to its default state.
+///
+/// - Strictly nothrow move constructible and assignable, which is also required of each
+/// of T...
+///
+/// - Less sophisticated type deduction. std::variant<bool, std::string>("hello") will
+/// intelligently construct std::string while Variant will construct bool.
+///
+/// - Either both copy constructible and assignable or neither (std::variant independently
+/// enables copy construction and copy assignment). Variant is copy constructible if
+/// each of T... is copy constructible and assignable.
+///
+/// - Slimmer interface; several members of std::variant are omitted.
+///
+/// - Throws no exceptions; if a bad_variant_access would be thrown Variant will instead
+/// segfault (nullptr dereference).
+///
+/// - Mutable visit takes a pointer instead of mutable reference or rvalue reference,
+/// which is more conformant with our code style.
+template <typename... T>
+class Variant;
+
+namespace detail {
+
+template <typename T, typename = void>
+struct is_equality_comparable : std::false_type {};
+
+template <typename T>
+struct is_equality_comparable<
+ T, typename std::enable_if<std::is_convertible<
+ decltype(std::declval<T>() == std::declval<T>()), bool>::value>::type>
+ : std::true_type {};
+
+template <bool C, typename T, typename E>
+using conditional_t = typename std::conditional<C, T, E>::type;
+
+template <typename T>
+struct type_constant {
+ using type = T;
+};
+
+template <typename...>
+struct first;
+
+template <typename H, typename... T>
+struct first<H, T...> {
+ using type = H;
+};
+
+template <typename T>
+using decay_t = typename std::decay<T>::type;
+
+template <bool...>
+struct all : std::true_type {};
+
+template <bool H, bool... T>
+struct all<H, T...> : conditional_t<H, all<T...>, std::false_type> {};
+
+template <typename T>
+void copy_construct(void* ptr, const void* other) {
+ new (ptr) T(*static_cast<const T*>(other));
+}
+
+template <typename T>
+void move_construct(void* ptr, void* other) {
+ new (ptr) T(std::move(*static_cast<T*>(other)));
+}
+
+template <typename T>
+void explicit_destroy(void* ptr) {
+ static_cast<T*>(ptr)->~T();
+}
+
+inline void trivial_destroy(void*) {}
+
+template <typename T>
+bool equal(const void* l, const void* r) {
+ return *static_cast<const T*>(l) == *static_cast<const T*>(r);
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<const T&>())) visit_const_ref(
+ Visitor&& visitor, const void* ptr) {
+ return std::forward<Visitor>(visitor)(*static_cast<const T*>(ptr));
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<T*>())) visit_mutable_ptr(
+ Visitor&& visitor, void* ptr) {
+ return std::forward<Visitor>(visitor)(static_cast<T*>(ptr));
+}
+
+template <typename... T>
+struct variant_storage {
+ variant_storage() = default;
+ variant_storage(const variant_storage&) {}
+ variant_storage& operator=(const variant_storage&) { return *this; }
+ variant_storage(variant_storage&&) {}
+ variant_storage& operator=(variant_storage&&) { return *this; }
+ ~variant_storage() {
+ static_assert(offsetof(variant_storage, data_) == 0,
+ "(void*)&variant_storage::data_ == (void*)this");
+ }
+
+ typename std::aligned_union<0, T...>::type data_;
+ uint8_t index_ = 0;
+};
+
+struct delete_copy_constructor {
+ template <typename>
+ struct type {
+ type() = default;
+ type(const type& other) = delete;
+ type& operator=(const type& other) = delete;
+ };
+};
+
+struct explicit_copy_constructor {
+ template <typename Copyable>
+ struct type {
+ type() = default;
+ type(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/false);
+ }
+ type& operator=(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/true);
+ return *this;
+ }
+ };
+};
+
+template <typename V, uint8_t I, typename...>
+struct member_constructor {
+ static void index_of() {}
+};
+
+template <typename V, uint8_t I, typename H, typename... T>
+struct member_constructor<V, I, H, T...> : member_constructor<V, I + 1, T...> {
+ member_constructor() = default;
+
+ using member_constructor<V, I + 1, T...>::member_constructor;
+ using member_constructor<V, I + 1, T...>::operator=;
+ using member_constructor<V, I + 1, T...>::index_of;
+
+ explicit member_constructor(H value) {
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ }
+
+ member_constructor& operator=(H value) {
+ static_cast<V*>(this)->destroy();
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ return *this;
+ }
+
+ static constexpr std::integral_constant<uint8_t, I> index_of(const type_constant<H>&) {
+ return {};
+ }
+};
+
+} // namespace detail
+
+template <typename... T>
+class Variant : detail::variant_storage<T...>,
+ detail::conditional_t<
+ detail::all<(std::is_copy_constructible<T>::value &&
+ std::is_copy_assignable<T>::value)...>::value,
+ detail::explicit_copy_constructor,
+ detail::delete_copy_constructor>::template type<Variant<T...>>,
+ detail::member_constructor<Variant<T...>, 0, T...> {
+ static_assert(detail::all<(std::is_nothrow_move_constructible<T>::value &&
+ std::is_nothrow_move_assignable<T>::value)...>::value,
+ "valueless_by_exception is not supported");
+
+ template <typename U>
+ static constexpr uint8_t index_of() {
+ return detail::member_constructor<Variant<T...>, 0, T...>::index_of(
+ detail::type_constant<U>{});
+ }
+
+ public:
+ using default_type = typename util::detail::first<T...>::type;
+ static_assert(std::is_nothrow_default_constructible<default_type>::value,
+ "valueless_by_exception, non-default constructible are not supported");
+
+ Variant() noexcept { construct_default(); }
+
+ Variant(const Variant& other) = default;
+ Variant& operator=(const Variant& other) = default;
+
+ using detail::member_constructor<Variant<T...>, 0, T...>::member_constructor;
+ using detail::member_constructor<Variant<T...>, 0, T...>::operator=;
+
+ Variant(Variant&& other) noexcept { move(&other, /*assignment=*/false); }
+
+ Variant& operator=(Variant&& other) noexcept {
+ move(&other, /*assignment=*/true);
+ return *this;
+ }
+
+ ~Variant() {
+ static_assert(offsetof(Variant, data_) == 0, "(void*)&Variant::data_ == (void*)this");
+ this->destroy();
+ }
+
+ uint8_t index() const noexcept { return this->index_; }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ const U* get() const noexcept {
+ return index() == I ? reinterpret_cast<const U*>(this) : NULLPTR;
+ }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ U* get() noexcept {
+ return index() == I ? reinterpret_cast<U*>(this) : NULLPTR;
+ }
+
+ template <typename U, typename... A, uint8_t I = index_of<U>()>
+ void emplace(A&&... args) try {
+ this->destroy();
+ new (this) U(std::forward<A>(args)...);
+ this->index_ = I;
+ } catch (...) {
+ construct_default();
+ throw;
+ }
+
+ void swap(Variant& other) noexcept { // NOLINT google-runtime-references
+ Variant tmp = std::move(other);
+ other = std::move(*this);
+ *this = std::move(tmp);
+ }
+
+ private:
+ void move(Variant* other, bool assignment) noexcept {
+ using impl_t = void(void*, void*);
+ static std::array<impl_t*, sizeof...(T)> impl = {detail::move_construct<T>...};
+
+ if (assignment) {
+ this->destroy();
+ }
+
+ auto index = other->index_;
+ impl[index](this, other);
Review comment:
You could do something like:
```c++
template <typename V, uint8_t I, typename H, typename... T>
struct member_constructor<V, I, H, T...> : member_constructor<V, I + 1, T...> {
// ...
void move_to(uint8_t index, Variant* target) {
if (index == I) {
detail::move_construct(target, this);
} else {
member_constructor<V, I + 1, T...>::move_to(index, target);
}
}
```
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #8472: ARROW-8113: [C++] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
kou commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-742143475
https://issues.apache.org/jira/browse/ARROW-10867 may be related.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-712950634
Updated description with build times and code sizes for a release build
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8472: ARROW-8113: [C++] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-735900749
AppVeyor seems to indicate that MSVC isn't picking up / implementing the copy constructor correctly :-(
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#discussion_r505758582
##########
File path: cpp/src/arrow/util/variant.h
##########
@@ -17,17 +17,363 @@
#pragma once
-#include "arrow/vendored/variant.hpp" // IWYU pragma: export
+#include <array>
+#include <cstddef>
+#include <type_traits>
+#include <utility>
+
+#include "arrow/util/macros.h"
namespace arrow {
namespace util {
-using ::mpark::bad_variant_access;
-using ::mpark::get;
-using ::mpark::get_if;
-using ::mpark::holds_alternative;
-using ::mpark::variant;
-using ::mpark::visit;
+/// \brief a std::variant-like discriminated union
+///
+/// Simplifications from std::variant:
+///
+/// - Strictly defaultable. The first type of T... must be nothrow default constructible
+/// and it will be used for default Variants.
+///
+/// - Never valueless_by_exception. std::variant supports a state outside those specified
+/// by T... to which it can return in the event that a constructor throws. If a Variant
+/// would become valueless_by_exception it will instead return to its default state.
+///
+/// - Strictly nothrow move constructible and assignable, which is also required of each
+/// of T...
+///
+/// - Less sophisticated type deduction. std::variant<bool, std::string>("hello") will
+/// intelligently construct std::string while Variant will construct bool.
+///
+/// - Either both copy constructible and assignable or neither (std::variant independently
+/// enables copy construction and copy assignment). Variant is copy constructible if
+/// each of T... is copy constructible and assignable.
+///
+/// - Slimmer interface; several members of std::variant are omitted.
+///
+/// - Throws no exceptions; if a bad_variant_access would be thrown Variant will instead
+/// segfault (nullptr dereference).
+///
+/// - Mutable visit takes a pointer instead of mutable reference or rvalue reference,
+/// which is more conformant with our code style.
+template <typename... T>
+class Variant;
+
+namespace detail {
+
+template <typename T, typename = void>
+struct is_equality_comparable : std::false_type {};
+
+template <typename T>
+struct is_equality_comparable<
+ T, typename std::enable_if<std::is_convertible<
+ decltype(std::declval<T>() == std::declval<T>()), bool>::value>::type>
+ : std::true_type {};
+
+template <bool C, typename T, typename E>
+using conditional_t = typename std::conditional<C, T, E>::type;
+
+template <typename T>
+struct type_constant {
+ using type = T;
+};
+
+template <typename...>
+struct first;
+
+template <typename H, typename... T>
+struct first<H, T...> {
+ using type = H;
+};
+
+template <typename T>
+using decay_t = typename std::decay<T>::type;
+
+template <bool...>
+struct all : std::true_type {};
+
+template <bool H, bool... T>
+struct all<H, T...> : conditional_t<H, all<T...>, std::false_type> {};
+
+template <typename T>
+void copy_construct(void* ptr, const void* other) {
+ new (ptr) T(*static_cast<const T*>(other));
+}
+
+template <typename T>
+void move_construct(void* ptr, void* other) {
+ new (ptr) T(std::move(*static_cast<T*>(other)));
+}
+
+template <typename T>
+void explicit_destroy(void* ptr) {
+ static_cast<T*>(ptr)->~T();
+}
+
+inline void trivial_destroy(void*) {}
+
+template <typename T>
+bool equal(const void* l, const void* r) {
+ return *static_cast<const T*>(l) == *static_cast<const T*>(r);
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<const T&>())) visit_const_ref(
+ Visitor&& visitor, const void* ptr) {
+ return std::forward<Visitor>(visitor)(*static_cast<const T*>(ptr));
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<T*>())) visit_mutable_ptr(
+ Visitor&& visitor, void* ptr) {
+ return std::forward<Visitor>(visitor)(static_cast<T*>(ptr));
+}
+
+template <typename... T>
+struct variant_storage {
+ variant_storage() = default;
+ variant_storage(const variant_storage&) {}
+ variant_storage& operator=(const variant_storage&) { return *this; }
+ variant_storage(variant_storage&&) {}
+ variant_storage& operator=(variant_storage&&) { return *this; }
+ ~variant_storage() {
+ static_assert(offsetof(variant_storage, data_) == 0,
+ "(void*)&variant_storage::data_ == (void*)this");
+ }
+
+ typename std::aligned_union<0, T...>::type data_;
+ uint8_t index_ = 0;
+};
+
+struct delete_copy_constructor {
+ template <typename>
+ struct type {
+ type() = default;
+ type(const type& other) = delete;
+ type& operator=(const type& other) = delete;
+ };
+};
+
+struct explicit_copy_constructor {
+ template <typename Copyable>
+ struct type {
+ type() = default;
+ type(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/false);
+ }
+ type& operator=(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/true);
+ return *this;
+ }
+ };
+};
+
+template <typename V, uint8_t I, typename...>
+struct member_constructor {
+ static void index_of() {}
+};
+
+template <typename V, uint8_t I, typename H, typename... T>
+struct member_constructor<V, I, H, T...> : member_constructor<V, I + 1, T...> {
+ member_constructor() = default;
+
+ using member_constructor<V, I + 1, T...>::member_constructor;
+ using member_constructor<V, I + 1, T...>::operator=;
+ using member_constructor<V, I + 1, T...>::index_of;
+
+ explicit member_constructor(H value) {
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ }
+
+ member_constructor& operator=(H value) {
+ static_cast<V*>(this)->destroy();
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ return *this;
+ }
+
+ static constexpr std::integral_constant<uint8_t, I> index_of(const type_constant<H>&) {
+ return {};
+ }
+};
+
+} // namespace detail
+
+template <typename... T>
+class Variant : detail::variant_storage<T...>,
+ detail::conditional_t<
+ detail::all<(std::is_copy_constructible<T>::value &&
+ std::is_copy_assignable<T>::value)...>::value,
+ detail::explicit_copy_constructor,
+ detail::delete_copy_constructor>::template type<Variant<T...>>,
+ detail::member_constructor<Variant<T...>, 0, T...> {
+ static_assert(detail::all<(std::is_nothrow_move_constructible<T>::value &&
+ std::is_nothrow_move_assignable<T>::value)...>::value,
+ "valueless_by_exception is not supported");
+
+ template <typename U>
+ static constexpr uint8_t index_of() {
+ return detail::member_constructor<Variant<T...>, 0, T...>::index_of(
+ detail::type_constant<U>{});
+ }
+
+ public:
+ using default_type = typename util::detail::first<T...>::type;
+ static_assert(std::is_nothrow_default_constructible<default_type>::value,
+ "valueless_by_exception, non-default constructible are not supported");
+
+ Variant() noexcept { construct_default(); }
+
+ Variant(const Variant& other) = default;
+ Variant& operator=(const Variant& other) = default;
+
+ using detail::member_constructor<Variant<T...>, 0, T...>::member_constructor;
+ using detail::member_constructor<Variant<T...>, 0, T...>::operator=;
+
+ Variant(Variant&& other) noexcept { move(&other, /*assignment=*/false); }
+
+ Variant& operator=(Variant&& other) noexcept {
+ move(&other, /*assignment=*/true);
+ return *this;
+ }
+
+ ~Variant() {
+ static_assert(offsetof(Variant, data_) == 0, "(void*)&Variant::data_ == (void*)this");
+ this->destroy();
+ }
+
+ uint8_t index() const noexcept { return this->index_; }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ const U* get() const noexcept {
+ return index() == I ? reinterpret_cast<const U*>(this) : NULLPTR;
+ }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ U* get() noexcept {
+ return index() == I ? reinterpret_cast<U*>(this) : NULLPTR;
+ }
+
+ template <typename U, typename... A, uint8_t I = index_of<U>()>
+ void emplace(A&&... args) try {
+ this->destroy();
+ new (this) U(std::forward<A>(args)...);
+ this->index_ = I;
+ } catch (...) {
+ construct_default();
+ throw;
+ }
+
+ void swap(Variant& other) noexcept { // NOLINT google-runtime-references
+ Variant tmp = std::move(other);
+ other = std::move(*this);
+ *this = std::move(tmp);
+ }
+
+ private:
+ void move(Variant* other, bool assignment) noexcept {
+ using impl_t = void(void*, void*);
+ static std::array<impl_t*, sizeof...(T)> impl = {detail::move_construct<T>...};
+
+ if (assignment) {
+ this->destroy();
+ }
+
+ auto index = other->index_;
+ impl[index](this, other);
Review comment:
Are you sure this kind of function table access will be inlined by the compiler as a switch/case?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-712233535
@emkornfield I'm implementing a subset of `std::variant`'s functionality but all members which are present are compatible; if we switch to using `std::variant` at some point no code will break. We will need to keep the mutable-pointer visit overload since that's non standard, but IMHO that's acceptable overhead
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #8472: ARROW-8113: [C++] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
kou commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-742023540
I don't know why but It seems that this causes g++ 9 crash:
https://github.com/ursa-labs/crossbow/runs/1522097163#step:4:1237
```text
during GIMPLE pass: pre
In file included from /usr/include/c++/9/functional:59,
from /arrow/cpp/src/arrow/vendored/string_view.hpp:1480,
from /arrow/cpp/src/arrow/util/string_view.h:25,
from /arrow/cpp/src/arrow/buffer.h:31,
from /arrow/cpp/src/arrow/array/data.h:26,
from /arrow/cpp/src/arrow/array/array_base.h:26,
from /arrow/cpp/src/arrow/array/builder_binary.h:30,
from /arrow/cpp/src/arrow/compute/kernels/codegen_internal.h:26,
from /arrow/cpp/src/arrow/compute/kernels/codegen_internal.cc:18:
/usr/include/c++/9/bits/std_function.h: In static member function 'static void std::_Function_handler<void(_ArgTypes ...), _Functor>::_M_invoke(const std::_Any_data&, _ArgTypes&& ...) [with _Functor = arrow::compute::internal::MakeFlippedBinaryExec(arrow::compute::ArrayKernelExec)::<lambda(arrow::compute::KernelContext*, const arrow::compute::ExecBatch&, arrow::Datum*)>; _ArgTypes = {arrow::compute::KernelContext*, const arrow::compute::ExecBatch&, arrow::Datum*}]':
/usr/include/c++/9/bits/std_function.h:298:7: internal compiler error: Segmentation fault
298 | _M_invoke(const _Any_data& __functor, _ArgTypes&&... __args)
| ^~~~~~~~~
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-9/README.Bugs> for instructions.
make[2]: *** [src/arrow/CMakeFiles/arrow_objlib.dir/build.make:1610: src/arrow/CMakeFiles/arrow_objlib.dir/compute/kernels/codegen_internal.cc.o] Error 1
```
We can reproduce this on local:
```bash
(cd cpp/examples/minimal_build && docker-compose build static && docker-compose run --rm static)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8472: ARROW-8113: [C++] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-735860929
Variant benchmarks:
* before:
```
ConstructTrivialVariant 13290 ns 13287 ns 52532 items_per_second=752.595M/s
ConstructNonTrivialVariant 111230 ns 111215 ns 6202 items_per_second=89.916M/s
VisitTrivialVariant 25744 ns 25740 ns 27269 items_per_second=388.497M/s
VisitNonTrivialVariant 24389 ns 24385 ns 28649 items_per_second=410.082M/s
ConstructDatum 102925 ns 102911 ns 7190 items_per_second=97.1714M/s
VisitDatum 6204 ns 6204 ns 112708 items_per_second=1.61199G/s
```
* after:
```
ConstructTrivialVariant 13952 ns 13951 ns 50116 items_per_second=716.811M/s
ConstructNonTrivialVariant 108507 ns 108489 ns 6412 items_per_second=92.1753M/s
VisitTrivialVariant 5848 ns 5847 ns 119628 items_per_second=1.71014G/s
VisitNonTrivialVariant 5973 ns 5972 ns 118440 items_per_second=1.6745G/s
ConstructDatum 98938 ns 98923 ns 7038 items_per_second=101.088M/s
VisitDatum 5566 ns 5565 ns 125741 items_per_second=1.79701G/s
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#discussion_r505778397
##########
File path: cpp/src/arrow/util/variant.h
##########
@@ -17,17 +17,363 @@
#pragma once
-#include "arrow/vendored/variant.hpp" // IWYU pragma: export
+#include <array>
+#include <cstddef>
+#include <type_traits>
+#include <utility>
+
+#include "arrow/util/macros.h"
namespace arrow {
namespace util {
-using ::mpark::bad_variant_access;
-using ::mpark::get;
-using ::mpark::get_if;
-using ::mpark::holds_alternative;
-using ::mpark::variant;
-using ::mpark::visit;
+/// \brief a std::variant-like discriminated union
+///
+/// Simplifications from std::variant:
+///
+/// - Strictly defaultable. The first type of T... must be nothrow default constructible
+/// and it will be used for default Variants.
+///
+/// - Never valueless_by_exception. std::variant supports a state outside those specified
+/// by T... to which it can return in the event that a constructor throws. If a Variant
+/// would become valueless_by_exception it will instead return to its default state.
+///
+/// - Strictly nothrow move constructible and assignable, which is also required of each
+/// of T...
+///
+/// - Less sophisticated type deduction. std::variant<bool, std::string>("hello") will
+/// intelligently construct std::string while Variant will construct bool.
+///
+/// - Either both copy constructible and assignable or neither (std::variant independently
+/// enables copy construction and copy assignment). Variant is copy constructible if
+/// each of T... is copy constructible and assignable.
+///
+/// - Slimmer interface; several members of std::variant are omitted.
+///
+/// - Throws no exceptions; if a bad_variant_access would be thrown Variant will instead
+/// segfault (nullptr dereference).
+///
+/// - Mutable visit takes a pointer instead of mutable reference or rvalue reference,
+/// which is more conformant with our code style.
+template <typename... T>
+class Variant;
+
+namespace detail {
+
+template <typename T, typename = void>
+struct is_equality_comparable : std::false_type {};
+
+template <typename T>
+struct is_equality_comparable<
+ T, typename std::enable_if<std::is_convertible<
+ decltype(std::declval<T>() == std::declval<T>()), bool>::value>::type>
+ : std::true_type {};
+
+template <bool C, typename T, typename E>
+using conditional_t = typename std::conditional<C, T, E>::type;
+
+template <typename T>
+struct type_constant {
+ using type = T;
+};
+
+template <typename...>
+struct first;
+
+template <typename H, typename... T>
+struct first<H, T...> {
+ using type = H;
+};
+
+template <typename T>
+using decay_t = typename std::decay<T>::type;
+
+template <bool...>
+struct all : std::true_type {};
+
+template <bool H, bool... T>
+struct all<H, T...> : conditional_t<H, all<T...>, std::false_type> {};
+
+template <typename T>
+void copy_construct(void* ptr, const void* other) {
+ new (ptr) T(*static_cast<const T*>(other));
+}
+
+template <typename T>
+void move_construct(void* ptr, void* other) {
+ new (ptr) T(std::move(*static_cast<T*>(other)));
+}
+
+template <typename T>
+void explicit_destroy(void* ptr) {
+ static_cast<T*>(ptr)->~T();
+}
+
+inline void trivial_destroy(void*) {}
+
+template <typename T>
+bool equal(const void* l, const void* r) {
+ return *static_cast<const T*>(l) == *static_cast<const T*>(r);
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<const T&>())) visit_const_ref(
+ Visitor&& visitor, const void* ptr) {
+ return std::forward<Visitor>(visitor)(*static_cast<const T*>(ptr));
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<T*>())) visit_mutable_ptr(
+ Visitor&& visitor, void* ptr) {
+ return std::forward<Visitor>(visitor)(static_cast<T*>(ptr));
+}
+
+template <typename... T>
+struct variant_storage {
+ variant_storage() = default;
+ variant_storage(const variant_storage&) {}
+ variant_storage& operator=(const variant_storage&) { return *this; }
+ variant_storage(variant_storage&&) {}
+ variant_storage& operator=(variant_storage&&) { return *this; }
+ ~variant_storage() {
+ static_assert(offsetof(variant_storage, data_) == 0,
+ "(void*)&variant_storage::data_ == (void*)this");
+ }
+
+ typename std::aligned_union<0, T...>::type data_;
+ uint8_t index_ = 0;
+};
+
+struct delete_copy_constructor {
+ template <typename>
+ struct type {
+ type() = default;
+ type(const type& other) = delete;
+ type& operator=(const type& other) = delete;
+ };
+};
+
+struct explicit_copy_constructor {
+ template <typename Copyable>
+ struct type {
+ type() = default;
+ type(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/false);
+ }
+ type& operator=(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/true);
+ return *this;
+ }
+ };
+};
+
+template <typename V, uint8_t I, typename...>
+struct member_constructor {
+ static void index_of() {}
+};
+
+template <typename V, uint8_t I, typename H, typename... T>
+struct member_constructor<V, I, H, T...> : member_constructor<V, I + 1, T...> {
+ member_constructor() = default;
+
+ using member_constructor<V, I + 1, T...>::member_constructor;
+ using member_constructor<V, I + 1, T...>::operator=;
+ using member_constructor<V, I + 1, T...>::index_of;
+
+ explicit member_constructor(H value) {
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ }
+
+ member_constructor& operator=(H value) {
+ static_cast<V*>(this)->destroy();
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ return *this;
+ }
+
+ static constexpr std::integral_constant<uint8_t, I> index_of(const type_constant<H>&) {
+ return {};
+ }
+};
+
+} // namespace detail
+
+template <typename... T>
+class Variant : detail::variant_storage<T...>,
+ detail::conditional_t<
+ detail::all<(std::is_copy_constructible<T>::value &&
+ std::is_copy_assignable<T>::value)...>::value,
+ detail::explicit_copy_constructor,
+ detail::delete_copy_constructor>::template type<Variant<T...>>,
+ detail::member_constructor<Variant<T...>, 0, T...> {
+ static_assert(detail::all<(std::is_nothrow_move_constructible<T>::value &&
+ std::is_nothrow_move_assignable<T>::value)...>::value,
+ "valueless_by_exception is not supported");
+
+ template <typename U>
+ static constexpr uint8_t index_of() {
+ return detail::member_constructor<Variant<T...>, 0, T...>::index_of(
+ detail::type_constant<U>{});
+ }
+
+ public:
+ using default_type = typename util::detail::first<T...>::type;
+ static_assert(std::is_nothrow_default_constructible<default_type>::value,
+ "valueless_by_exception, non-default constructible are not supported");
+
+ Variant() noexcept { construct_default(); }
+
+ Variant(const Variant& other) = default;
+ Variant& operator=(const Variant& other) = default;
+
+ using detail::member_constructor<Variant<T...>, 0, T...>::member_constructor;
+ using detail::member_constructor<Variant<T...>, 0, T...>::operator=;
+
+ Variant(Variant&& other) noexcept { move(&other, /*assignment=*/false); }
+
+ Variant& operator=(Variant&& other) noexcept {
+ move(&other, /*assignment=*/true);
+ return *this;
+ }
+
+ ~Variant() {
+ static_assert(offsetof(Variant, data_) == 0, "(void*)&Variant::data_ == (void*)this");
+ this->destroy();
+ }
+
+ uint8_t index() const noexcept { return this->index_; }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ const U* get() const noexcept {
+ return index() == I ? reinterpret_cast<const U*>(this) : NULLPTR;
+ }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ U* get() noexcept {
+ return index() == I ? reinterpret_cast<U*>(this) : NULLPTR;
+ }
+
+ template <typename U, typename... A, uint8_t I = index_of<U>()>
+ void emplace(A&&... args) try {
+ this->destroy();
+ new (this) U(std::forward<A>(args)...);
+ this->index_ = I;
+ } catch (...) {
+ construct_default();
+ throw;
+ }
+
+ void swap(Variant& other) noexcept { // NOLINT google-runtime-references
+ Variant tmp = std::move(other);
+ other = std::move(*this);
+ *this = std::move(tmp);
+ }
+
+ private:
+ void move(Variant* other, bool assignment) noexcept {
+ using impl_t = void(void*, void*);
+ static std::array<impl_t*, sizeof...(T)> impl = {detail::move_construct<T>...};
+
+ if (assignment) {
+ this->destroy();
+ }
+
+ auto index = other->index_;
+ impl[index](this, other);
Review comment:
You could do something like:
```c++
template <typename V, uint8_t I, typename H, typename... T>
struct member_constructor<V, I, H, T...> : member_constructor<V, I + 1, T...> {
// ...
void move_to(uint8_t index, Variant* target) {
if (index == I) {
detail::move_construct(target, this);
} else {
member_constructor<V, I + 1, T...>::move_to(index, target);
}
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#discussion_r506498924
##########
File path: cpp/src/arrow/util/variant.h
##########
@@ -17,17 +17,363 @@
#pragma once
-#include "arrow/vendored/variant.hpp" // IWYU pragma: export
+#include <array>
+#include <cstddef>
+#include <type_traits>
+#include <utility>
+
+#include "arrow/util/macros.h"
namespace arrow {
namespace util {
-using ::mpark::bad_variant_access;
-using ::mpark::get;
-using ::mpark::get_if;
-using ::mpark::holds_alternative;
-using ::mpark::variant;
-using ::mpark::visit;
+/// \brief a std::variant-like discriminated union
+///
+/// Simplifications from std::variant:
+///
+/// - Strictly defaultable. The first type of T... must be nothrow default constructible
+/// and it will be used for default Variants.
+///
+/// - Never valueless_by_exception. std::variant supports a state outside those specified
+/// by T... to which it can return in the event that a constructor throws. If a Variant
+/// would become valueless_by_exception it will instead return to its default state.
+///
+/// - Strictly nothrow move constructible and assignable, which is also required of each
+/// of T...
+///
+/// - Less sophisticated type deduction. std::variant<bool, std::string>("hello") will
+/// intelligently construct std::string while Variant will construct bool.
+///
+/// - Either both copy constructible and assignable or neither (std::variant independently
+/// enables copy construction and copy assignment). Variant is copy constructible if
+/// each of T... is copy constructible and assignable.
+///
+/// - Slimmer interface; several members of std::variant are omitted.
+///
+/// - Throws no exceptions; if a bad_variant_access would be thrown Variant will instead
+/// segfault (nullptr dereference).
+///
+/// - Mutable visit takes a pointer instead of mutable reference or rvalue reference,
+/// which is more conformant with our code style.
+template <typename... T>
+class Variant;
+
+namespace detail {
+
+template <typename T, typename = void>
+struct is_equality_comparable : std::false_type {};
+
+template <typename T>
+struct is_equality_comparable<
+ T, typename std::enable_if<std::is_convertible<
+ decltype(std::declval<T>() == std::declval<T>()), bool>::value>::type>
+ : std::true_type {};
+
+template <bool C, typename T, typename E>
+using conditional_t = typename std::conditional<C, T, E>::type;
+
+template <typename T>
+struct type_constant {
+ using type = T;
+};
+
+template <typename...>
+struct first;
+
+template <typename H, typename... T>
+struct first<H, T...> {
+ using type = H;
+};
+
+template <typename T>
+using decay_t = typename std::decay<T>::type;
+
+template <bool...>
+struct all : std::true_type {};
+
+template <bool H, bool... T>
+struct all<H, T...> : conditional_t<H, all<T...>, std::false_type> {};
+
+template <typename T>
+void copy_construct(void* ptr, const void* other) {
+ new (ptr) T(*static_cast<const T*>(other));
+}
+
+template <typename T>
+void move_construct(void* ptr, void* other) {
+ new (ptr) T(std::move(*static_cast<T*>(other)));
+}
+
+template <typename T>
+void explicit_destroy(void* ptr) {
+ static_cast<T*>(ptr)->~T();
+}
+
+inline void trivial_destroy(void*) {}
+
+template <typename T>
+bool equal(const void* l, const void* r) {
+ return *static_cast<const T*>(l) == *static_cast<const T*>(r);
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<const T&>())) visit_const_ref(
+ Visitor&& visitor, const void* ptr) {
+ return std::forward<Visitor>(visitor)(*static_cast<const T*>(ptr));
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<T*>())) visit_mutable_ptr(
+ Visitor&& visitor, void* ptr) {
+ return std::forward<Visitor>(visitor)(static_cast<T*>(ptr));
+}
+
+template <typename... T>
+struct variant_storage {
+ variant_storage() = default;
+ variant_storage(const variant_storage&) {}
+ variant_storage& operator=(const variant_storage&) { return *this; }
+ variant_storage(variant_storage&&) {}
+ variant_storage& operator=(variant_storage&&) { return *this; }
+ ~variant_storage() {
+ static_assert(offsetof(variant_storage, data_) == 0,
+ "(void*)&variant_storage::data_ == (void*)this");
+ }
+
+ typename std::aligned_union<0, T...>::type data_;
+ uint8_t index_ = 0;
+};
+
+struct delete_copy_constructor {
+ template <typename>
+ struct type {
+ type() = default;
+ type(const type& other) = delete;
+ type& operator=(const type& other) = delete;
+ };
+};
+
+struct explicit_copy_constructor {
+ template <typename Copyable>
+ struct type {
+ type() = default;
+ type(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/false);
+ }
+ type& operator=(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/true);
+ return *this;
+ }
+ };
+};
+
+template <typename V, uint8_t I, typename...>
+struct member_constructor {
+ static void index_of() {}
+};
+
+template <typename V, uint8_t I, typename H, typename... T>
+struct member_constructor<V, I, H, T...> : member_constructor<V, I + 1, T...> {
+ static_assert(std::is_nothrow_move_constructible<H>::value &&
+ std::is_nothrow_move_assignable<H>::value,
+ "All members must be nothrow move constructible and assignable.");
+
+ member_constructor() = default;
+
+ using member_constructor<V, I + 1, T...>::member_constructor;
+ using member_constructor<V, I + 1, T...>::operator=;
+ using member_constructor<V, I + 1, T...>::index_of;
+
+ explicit member_constructor(H value) {
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ }
+
+ member_constructor& operator=(H value) {
+ static_cast<V*>(this)->destroy();
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ return *this;
+ }
+
+ static constexpr std::integral_constant<uint8_t, I> index_of(const type_constant<H>&) {
+ return {};
+ }
+};
+
+} // namespace detail
+
+template <typename... T>
+class Variant : detail::variant_storage<T...>,
+ detail::conditional_t<
+ detail::all<(std::is_copy_constructible<T>::value &&
+ std::is_copy_assignable<T>::value)...>::value,
+ detail::explicit_copy_constructor,
+ detail::delete_copy_constructor>::template type<Variant<T...>>,
Review comment:
We might want to borrow this idiom to disable copy construction for `Result<std::unique_ptr<T>>`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#issuecomment-711116750
drive by comment. just want to make sure we aren't using the same idioms for variant (i.e. they are compatible with C++ std)?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8472: ARROW-8113: [C++][WIP] Lighter weight variant<>
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8472:
URL: https://github.com/apache/arrow/pull/8472#discussion_r505758011
##########
File path: cpp/src/arrow/util/variant.h
##########
@@ -17,17 +17,363 @@
#pragma once
-#include "arrow/vendored/variant.hpp" // IWYU pragma: export
+#include <array>
+#include <cstddef>
+#include <type_traits>
+#include <utility>
+
+#include "arrow/util/macros.h"
namespace arrow {
namespace util {
-using ::mpark::bad_variant_access;
-using ::mpark::get;
-using ::mpark::get_if;
-using ::mpark::holds_alternative;
-using ::mpark::variant;
-using ::mpark::visit;
+/// \brief a std::variant-like discriminated union
+///
+/// Simplifications from std::variant:
+///
+/// - Strictly defaultable. The first type of T... must be nothrow default constructible
+/// and it will be used for default Variants.
+///
+/// - Never valueless_by_exception. std::variant supports a state outside those specified
+/// by T... to which it can return in the event that a constructor throws. If a Variant
+/// would become valueless_by_exception it will instead return to its default state.
+///
+/// - Strictly nothrow move constructible and assignable, which is also required of each
+/// of T...
+///
+/// - Less sophisticated type deduction. std::variant<bool, std::string>("hello") will
+/// intelligently construct std::string while Variant will construct bool.
+///
+/// - Either both copy constructible and assignable or neither (std::variant independently
+/// enables copy construction and copy assignment). Variant is copy constructible if
+/// each of T... is copy constructible and assignable.
+///
+/// - Slimmer interface; several members of std::variant are omitted.
+///
+/// - Throws no exceptions; if a bad_variant_access would be thrown Variant will instead
+/// segfault (nullptr dereference).
+///
+/// - Mutable visit takes a pointer instead of mutable reference or rvalue reference,
+/// which is more conformant with our code style.
+template <typename... T>
+class Variant;
+
+namespace detail {
+
+template <typename T, typename = void>
+struct is_equality_comparable : std::false_type {};
+
+template <typename T>
+struct is_equality_comparable<
+ T, typename std::enable_if<std::is_convertible<
+ decltype(std::declval<T>() == std::declval<T>()), bool>::value>::type>
+ : std::true_type {};
+
+template <bool C, typename T, typename E>
+using conditional_t = typename std::conditional<C, T, E>::type;
+
+template <typename T>
+struct type_constant {
+ using type = T;
+};
+
+template <typename...>
+struct first;
+
+template <typename H, typename... T>
+struct first<H, T...> {
+ using type = H;
+};
+
+template <typename T>
+using decay_t = typename std::decay<T>::type;
+
+template <bool...>
+struct all : std::true_type {};
+
+template <bool H, bool... T>
+struct all<H, T...> : conditional_t<H, all<T...>, std::false_type> {};
+
+template <typename T>
+void copy_construct(void* ptr, const void* other) {
+ new (ptr) T(*static_cast<const T*>(other));
+}
+
+template <typename T>
+void move_construct(void* ptr, void* other) {
+ new (ptr) T(std::move(*static_cast<T*>(other)));
+}
+
+template <typename T>
+void explicit_destroy(void* ptr) {
+ static_cast<T*>(ptr)->~T();
+}
+
+inline void trivial_destroy(void*) {}
+
+template <typename T>
+bool equal(const void* l, const void* r) {
+ return *static_cast<const T*>(l) == *static_cast<const T*>(r);
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<const T&>())) visit_const_ref(
+ Visitor&& visitor, const void* ptr) {
+ return std::forward<Visitor>(visitor)(*static_cast<const T*>(ptr));
+}
+
+template <typename Visitor, typename T>
+decltype(std::declval<Visitor&&>()(std::declval<T*>())) visit_mutable_ptr(
+ Visitor&& visitor, void* ptr) {
+ return std::forward<Visitor>(visitor)(static_cast<T*>(ptr));
+}
+
+template <typename... T>
+struct variant_storage {
+ variant_storage() = default;
+ variant_storage(const variant_storage&) {}
+ variant_storage& operator=(const variant_storage&) { return *this; }
+ variant_storage(variant_storage&&) {}
+ variant_storage& operator=(variant_storage&&) { return *this; }
+ ~variant_storage() {
+ static_assert(offsetof(variant_storage, data_) == 0,
+ "(void*)&variant_storage::data_ == (void*)this");
+ }
+
+ typename std::aligned_union<0, T...>::type data_;
+ uint8_t index_ = 0;
+};
+
+struct delete_copy_constructor {
+ template <typename>
+ struct type {
+ type() = default;
+ type(const type& other) = delete;
+ type& operator=(const type& other) = delete;
+ };
+};
+
+struct explicit_copy_constructor {
+ template <typename Copyable>
+ struct type {
+ type() = default;
+ type(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/false);
+ }
+ type& operator=(const type& other) {
+ static_cast<Copyable*>(this)->copy(other, /*assignment=*/true);
+ return *this;
+ }
+ };
+};
+
+template <typename V, uint8_t I, typename...>
+struct member_constructor {
+ static void index_of() {}
+};
+
+template <typename V, uint8_t I, typename H, typename... T>
+struct member_constructor<V, I, H, T...> : member_constructor<V, I + 1, T...> {
+ member_constructor() = default;
+
+ using member_constructor<V, I + 1, T...>::member_constructor;
+ using member_constructor<V, I + 1, T...>::operator=;
+ using member_constructor<V, I + 1, T...>::index_of;
+
+ explicit member_constructor(H value) {
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ }
+
+ member_constructor& operator=(H value) {
+ static_cast<V*>(this)->destroy();
+ new (this) H(std::move(value));
+ static_cast<V*>(this)->index_ = I;
+ return *this;
+ }
+
+ static constexpr std::integral_constant<uint8_t, I> index_of(const type_constant<H>&) {
+ return {};
+ }
+};
+
+} // namespace detail
+
+template <typename... T>
+class Variant : detail::variant_storage<T...>,
+ detail::conditional_t<
+ detail::all<(std::is_copy_constructible<T>::value &&
+ std::is_copy_assignable<T>::value)...>::value,
+ detail::explicit_copy_constructor,
+ detail::delete_copy_constructor>::template type<Variant<T...>>,
+ detail::member_constructor<Variant<T...>, 0, T...> {
+ static_assert(detail::all<(std::is_nothrow_move_constructible<T>::value &&
+ std::is_nothrow_move_assignable<T>::value)...>::value,
+ "valueless_by_exception is not supported");
+
+ template <typename U>
+ static constexpr uint8_t index_of() {
+ return detail::member_constructor<Variant<T...>, 0, T...>::index_of(
+ detail::type_constant<U>{});
+ }
+
+ public:
+ using default_type = typename util::detail::first<T...>::type;
+ static_assert(std::is_nothrow_default_constructible<default_type>::value,
+ "valueless_by_exception, non-default constructible are not supported");
+
+ Variant() noexcept { construct_default(); }
+
+ Variant(const Variant& other) = default;
+ Variant& operator=(const Variant& other) = default;
+
+ using detail::member_constructor<Variant<T...>, 0, T...>::member_constructor;
+ using detail::member_constructor<Variant<T...>, 0, T...>::operator=;
+
+ Variant(Variant&& other) noexcept { move(&other, /*assignment=*/false); }
+
+ Variant& operator=(Variant&& other) noexcept {
+ move(&other, /*assignment=*/true);
+ return *this;
+ }
+
+ ~Variant() {
+ static_assert(offsetof(Variant, data_) == 0, "(void*)&Variant::data_ == (void*)this");
+ this->destroy();
+ }
+
+ uint8_t index() const noexcept { return this->index_; }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ const U* get() const noexcept {
+ return index() == I ? reinterpret_cast<const U*>(this) : NULLPTR;
+ }
+
+ template <typename U, uint8_t I = index_of<U>()>
+ U* get() noexcept {
+ return index() == I ? reinterpret_cast<U*>(this) : NULLPTR;
+ }
+
+ template <typename U, typename... A, uint8_t I = index_of<U>()>
+ void emplace(A&&... args) try {
+ this->destroy();
+ new (this) U(std::forward<A>(args)...);
+ this->index_ = I;
+ } catch (...) {
+ construct_default();
+ throw;
+ }
+
+ void swap(Variant& other) noexcept { // NOLINT google-runtime-references
+ Variant tmp = std::move(other);
+ other = std::move(*this);
+ *this = std::move(tmp);
+ }
+
+ private:
+ void move(Variant* other, bool assignment) noexcept {
+ using impl_t = void(void*, void*);
+ static std::array<impl_t*, sizeof...(T)> impl = {detail::move_construct<T>...};
Review comment:
`constexpr`, no?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org