bkietz commented on a change in pull request #7819: URL: https://github.com/apache/arrow/pull/7819#discussion_r460212984
########## File path: r/src/array_from_vector.cpp ########## @@ -406,9 +406,12 @@ std::shared_ptr<Array> MakeFactorArray(Rcpp::IntegerVector_ factor, case Type::INT64: return MakeFactorArrayImpl<arrow::Int64Type>(factor, type); default: - Rcpp::stop(tfm::format("Cannot convert to dictionary with index_type %s", - dict_type.index_type()->ToString())); + break; } + + cpp11::stop("Cannot convert to dictionary with index_type '", + dict_type.index_type()->ToString().c_str(), "'"); Review comment: IIUC, `cpp11::stop` still uses printf-esque syntax ```suggestion cpp11::stop("Cannot convert to dictionary with index_type '%s'", dict_type.index_type()->ToString().c_str()); ``` ########## File path: r/src/arrow_rcpp.h ########## @@ -63,124 +68,222 @@ struct ns { static SEXP arrow; }; +class Index { + public: + // TODO: uncomment when this is merged in cpp11: https://github.com/r-lib/cpp11/pull/58 + /*explicit*/ Index(SEXP x) : index_(validate_index(x)) {} // NOLINT runtime/explicit + + inline operator R_xlen_t() const { return index_; } + + private: + R_xlen_t index_; + + static R_xlen_t validate_index(SEXP x) { + if (XLENGTH(x) == 1) { + switch (TYPEOF(x)) { + case INTSXP: + return INTEGER_ELT(x, 0); + case REALSXP: + if (cpp11::is_convertable_without_loss_to_integer(REAL_ELT(x, 0))) + return REAL_ELT(x, 0); + case LGLSXP: + return LOGICAL_ELT(x, 0); + default: + break; + } + } + + cpp11::stop("Expected single integer value"); + return 0; + } +}; + } // namespace r } // namespace arrow namespace Rcpp { -namespace internal { +using NumericVector_ = Rcpp::Vector<REALSXP, Rcpp::NoProtectStorage>; +using IntegerVector_ = Rcpp::Vector<INTSXP, Rcpp::NoProtectStorage>; +using LogicalVector_ = Rcpp::Vector<LGLSXP, Rcpp::NoProtectStorage>; +using StringVector_ = Rcpp::Vector<STRSXP, Rcpp::NoProtectStorage>; +using CharacterVector_ = StringVector_; +using RawVector_ = Rcpp::Vector<RAWSXP, Rcpp::NoProtectStorage>; +using List_ = Rcpp::Vector<VECSXP, Rcpp::NoProtectStorage>; +} // namespace Rcpp + +namespace cpp11 { + +template <typename E> +typename std::enable_if<std::is_enum<E>::value, E>::type as_cpp(SEXP from) { + return E(cpp11::as_cpp<int>(from)); +} + +} // namespace cpp11 + +namespace arrow { +namespace r { template <typename Pointer> -Pointer r6_to_smart_pointer(SEXP self) { +Pointer r6_to_pointer(SEXP self) { return reinterpret_cast<Pointer>( R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp))); } -} // namespace internal - -template <typename T> -class ConstReferenceSmartPtrInputParameter { +template <typename T, template <class> class SmartPtr> +class ConstRefSmartPtrInput { public: - using const_reference = const T&; + using const_reference = const SmartPtr<T>&; - explicit ConstReferenceSmartPtrInputParameter(SEXP self) - : ptr(internal::r6_to_smart_pointer<const T*>(self)) {} + explicit ConstRefSmartPtrInput(SEXP self) + : ptr(r6_to_pointer<const SmartPtr<T>*>(self)) {} inline operator const_reference() { return *ptr; } private: - const T* ptr; + // this class host + const SmartPtr<T>* ptr; }; -template <typename T> -class ConstReferenceVectorSmartPtrInputParameter { +template <typename T, template <class> class SmartPtr> +class ConstRefVectorSmartPtrInput { public: - using const_reference = const std::vector<T>&; + using const_reference = const std::vector<SmartPtr<T>>&; - explicit ConstReferenceVectorSmartPtrInputParameter(SEXP self) : vec() { + explicit ConstRefVectorSmartPtrInput(SEXP self) : vec() { R_xlen_t n = XLENGTH(self); for (R_xlen_t i = 0; i < n; i++) { - vec.push_back(*internal::r6_to_smart_pointer<const T*>(VECTOR_ELT(self, i))); + vec.push_back(*r6_to_pointer<const SmartPtr<T>*>(VECTOR_ELT(self, i))); } } inline operator const_reference() { return vec; } private: - std::vector<T> vec; + std::vector<SmartPtr<T>> vec; +}; + +template <typename T> +class default_input { + public: + explicit default_input(SEXP from) : from_(from) {} + + operator T() const { return cpp11::as_cpp<T>(from_); } + + private: + SEXP from_; }; -namespace traits { +template <typename T> +class const_reference_input { + public: + explicit const_reference_input(SEXP from) : obj_(cpp11::as_cpp<T>(from)) {} + + using const_reference = const T&; + operator const_reference() const { return obj_; } + + private: + T obj_; +}; template <typename T> -struct input_parameter<const std::shared_ptr<T>&> { - typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<T>> type; +struct input { + using type = default_input<T>; }; template <typename T> -struct input_parameter<const std::unique_ptr<T>&> { - typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::unique_ptr<T>> type; +struct input<const T&> { + using type = const_reference_input<typename std::decay<T>::type>; }; template <typename T> -struct input_parameter<const std::vector<std::shared_ptr<T>>&> { - typedef typename Rcpp::ConstReferenceVectorSmartPtrInputParameter<std::shared_ptr<T>> - type; +struct input<const std::shared_ptr<T>&> { + using type = ConstRefSmartPtrInput<T, std::shared_ptr>; }; -struct wrap_type_shared_ptr_tag {}; -struct wrap_type_unique_ptr_tag {}; +template <typename T> +using default_unique_ptr = std::unique_ptr<T, std::default_delete<T>>; template <typename T> -struct wrap_type_traits<std::shared_ptr<T>> { - using wrap_category = wrap_type_shared_ptr_tag; +struct input<const std::unique_ptr<T>&> { + using type = ConstRefSmartPtrInput<T, default_unique_ptr>; }; template <typename T> -struct wrap_type_traits<std::unique_ptr<T>> { - using wrap_category = wrap_type_unique_ptr_tag; +struct input<const std::vector<std::shared_ptr<T>>&> { + using type = ConstRefVectorSmartPtrInput<T, std::shared_ptr>; }; -} // namespace traits +} // namespace r +} // namespace arrow -namespace internal { +namespace cpp11 { template <typename T> -inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_shared_ptr_tag); +using enable_if_shared_ptr = typename std::enable_if< + std::is_same<std::shared_ptr<typename T::element_type>, T>::value, T>::type; template <typename T> -inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_unique_ptr_tag); +enable_if_shared_ptr<T> as_cpp(SEXP from) { + return arrow::r::ConstRefSmartPtrInput<typename T::element_type, std::shared_ptr>(from); +} -} // namespace internal -} // namespace Rcpp +template <typename T> +SEXP as_sexp(const std::shared_ptr<T>& ptr) { + return Rcpp::XPtr<std::shared_ptr<T>>(new std::shared_ptr<T>(ptr)); +} -#include <Rcpp.h> +template <typename T> +cpp11::list as_sexp(const std::vector<std::shared_ptr<T>>& vec) { + R_xlen_t n = vec.size(); + SEXP res = PROTECT(Rf_allocVector(VECSXP, n)); + for (R_xlen_t i = 0; i < n; i++) { + SET_VECTOR_ELT(res, i, as_sexp(vec[i])); + } + UNPROTECT(1); + return res; +} -namespace Rcpp { -namespace internal { +template <typename E, typename std::enable_if<std::is_enum<E>::value>::type* = nullptr> +SEXP as_sexp(E e) { + return as_sexp(static_cast<int>(e)); +} -template <typename T> -inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_shared_ptr_tag) { - return Rcpp::XPtr<std::shared_ptr<typename T::element_type>>( - new std::shared_ptr<typename T::element_type>(x)); +} // namespace cpp11 + +namespace arrow { +namespace r { + +template <typename T, typename r_vec, typename Lambda> +r_vec to_r_vector(const std::vector<std::shared_ptr<T>>& x, Lambda lambda) { + auto n = x.size(); + r_vec out(n); + for (int i = 0; i < n; i++) { + out[i] = lambda(x[i]); + } + return out; } Review comment: The input vector's element type and the lambda type can be inferred, so the output type should be the first template parameter (look at `make_shared<T, A...>(A&&...)` for example). Also please use perfect capture for lambdas as copying them is not guaranteed to be cheap or defined. ```suggestion template <typename Rvector, typename T, typename ToVectorElement> r_vec to_r_vector(const std::vector<std::shared_ptr<T>>& x, ToVectorElement&& to_element) { auto n = x.size(); r_vec out(n); for (int i = 0; i < n; i++) { out[i] = to_element(x[i]); } return out; } ``` ########## File path: r/src/schema.cpp ########## @@ -72,47 +73,45 @@ bool Schema__HasMetadata(const std::shared_ptr<arrow::Schema>& schema) { } // [[arrow::export]] -Rcpp::List Schema__metadata(const std::shared_ptr<arrow::Schema>& schema) { +cpp11::writable::list Schema__metadata(const std::shared_ptr<arrow::Schema>& schema) { auto meta = schema->metadata(); int64_t n = 0; if (schema->HasMetadata()) { n = meta->size(); } - Rcpp::List out(n); + cpp11::writable::list out(n); std::vector<std::string> names_out(n); for (int i = 0; i < n; i++) { auto key = meta->key(i); - out[i] = meta->value(i); + out[i] = cpp11::as_sexp(meta->value(i)); if (key == "r") { - Rf_setAttrib(out[i], R_ClassSymbol, arrow::r::data::classes_metadata_r); + Rf_classgets(out[i], arrow::r::data::classes_metadata_r); } names_out[i] = key; } - out.attr("names") = names_out; + out.names() = names_out; return out; } // [[arrow::export]] std::shared_ptr<arrow::Schema> Schema__WithMetadata( - const std::shared_ptr<arrow::Schema>& schema, Rcpp::CharacterVector metadata) { - auto kv = std::shared_ptr<arrow::KeyValueMetadata>(new arrow::KeyValueMetadata( - metadata.names(), Rcpp::as<std::vector<std::string>>(metadata))); + const std::shared_ptr<arrow::Schema>& schema, cpp11::strings metadata) { + auto metadata_strings = cpp11::as_cpp<std::vector<std::string>>(metadata); + auto metadata_names = cpp11::as_cpp<std::vector<std::string>>(metadata.attr("names")); + + auto kv = std::shared_ptr<arrow::KeyValueMetadata>( + new arrow::KeyValueMetadata(metadata_names, metadata_strings)); return schema->WithMetadata(kv); Review comment: ```suggestion const std::shared_ptr<arrow::Schema>& schema, cpp11::strings metadata) { auto values = cpp11::as_cpp<std::vector<std::string>>(metadata); auto names = cpp11::as_cpp<std::vector<std::string>>(metadata.attr("names")); auto kv = std::make_shared<arrow::KeyValueMetadata>(std::move(names), std::move(values)); return schema->WithMetadata(std::move(kv)); ``` ``` ########## File path: r/src/arrow_rcpp.h ########## @@ -63,124 +68,222 @@ struct ns { static SEXP arrow; }; +class Index { + public: + // TODO: uncomment when this is merged in cpp11: https://github.com/r-lib/cpp11/pull/58 + /*explicit*/ Index(SEXP x) : index_(validate_index(x)) {} // NOLINT runtime/explicit + + inline operator R_xlen_t() const { return index_; } + + private: + R_xlen_t index_; + + static R_xlen_t validate_index(SEXP x) { + if (XLENGTH(x) == 1) { + switch (TYPEOF(x)) { + case INTSXP: + return INTEGER_ELT(x, 0); + case REALSXP: + if (cpp11::is_convertable_without_loss_to_integer(REAL_ELT(x, 0))) + return REAL_ELT(x, 0); + case LGLSXP: + return LOGICAL_ELT(x, 0); + default: + break; + } + } + + cpp11::stop("Expected single integer value"); + return 0; + } +}; + } // namespace r } // namespace arrow namespace Rcpp { -namespace internal { +using NumericVector_ = Rcpp::Vector<REALSXP, Rcpp::NoProtectStorage>; +using IntegerVector_ = Rcpp::Vector<INTSXP, Rcpp::NoProtectStorage>; +using LogicalVector_ = Rcpp::Vector<LGLSXP, Rcpp::NoProtectStorage>; +using StringVector_ = Rcpp::Vector<STRSXP, Rcpp::NoProtectStorage>; +using CharacterVector_ = StringVector_; +using RawVector_ = Rcpp::Vector<RAWSXP, Rcpp::NoProtectStorage>; +using List_ = Rcpp::Vector<VECSXP, Rcpp::NoProtectStorage>; +} // namespace Rcpp + +namespace cpp11 { + +template <typename E> +typename std::enable_if<std::is_enum<E>::value, E>::type as_cpp(SEXP from) { + return E(cpp11::as_cpp<int>(from)); +} + +} // namespace cpp11 + +namespace arrow { +namespace r { template <typename Pointer> -Pointer r6_to_smart_pointer(SEXP self) { +Pointer r6_to_pointer(SEXP self) { return reinterpret_cast<Pointer>( R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp))); } -} // namespace internal - -template <typename T> -class ConstReferenceSmartPtrInputParameter { +template <typename T, template <class> class SmartPtr> +class ConstRefSmartPtrInput { public: - using const_reference = const T&; + using const_reference = const SmartPtr<T>&; - explicit ConstReferenceSmartPtrInputParameter(SEXP self) - : ptr(internal::r6_to_smart_pointer<const T*>(self)) {} + explicit ConstRefSmartPtrInput(SEXP self) + : ptr(r6_to_pointer<const SmartPtr<T>*>(self)) {} inline operator const_reference() { return *ptr; } private: - const T* ptr; + // this class host + const SmartPtr<T>* ptr; }; -template <typename T> -class ConstReferenceVectorSmartPtrInputParameter { +template <typename T, template <class> class SmartPtr> +class ConstRefVectorSmartPtrInput { public: - using const_reference = const std::vector<T>&; + using const_reference = const std::vector<SmartPtr<T>>&; - explicit ConstReferenceVectorSmartPtrInputParameter(SEXP self) : vec() { + explicit ConstRefVectorSmartPtrInput(SEXP self) : vec() { R_xlen_t n = XLENGTH(self); for (R_xlen_t i = 0; i < n; i++) { - vec.push_back(*internal::r6_to_smart_pointer<const T*>(VECTOR_ELT(self, i))); + vec.push_back(*r6_to_pointer<const SmartPtr<T>*>(VECTOR_ELT(self, i))); } } inline operator const_reference() { return vec; } private: - std::vector<T> vec; + std::vector<SmartPtr<T>> vec; +}; + +template <typename T> +class default_input { + public: + explicit default_input(SEXP from) : from_(from) {} + + operator T() const { return cpp11::as_cpp<T>(from_); } + + private: + SEXP from_; }; -namespace traits { +template <typename T> +class const_reference_input { + public: + explicit const_reference_input(SEXP from) : obj_(cpp11::as_cpp<T>(from)) {} + + using const_reference = const T&; + operator const_reference() const { return obj_; } + + private: + T obj_; +}; template <typename T> -struct input_parameter<const std::shared_ptr<T>&> { - typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<T>> type; +struct input { + using type = default_input<T>; }; template <typename T> -struct input_parameter<const std::unique_ptr<T>&> { - typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::unique_ptr<T>> type; +struct input<const T&> { + using type = const_reference_input<typename std::decay<T>::type>; }; template <typename T> -struct input_parameter<const std::vector<std::shared_ptr<T>>&> { - typedef typename Rcpp::ConstReferenceVectorSmartPtrInputParameter<std::shared_ptr<T>> - type; +struct input<const std::shared_ptr<T>&> { + using type = ConstRefSmartPtrInput<T, std::shared_ptr>; }; -struct wrap_type_shared_ptr_tag {}; -struct wrap_type_unique_ptr_tag {}; +template <typename T> +using default_unique_ptr = std::unique_ptr<T, std::default_delete<T>>; template <typename T> -struct wrap_type_traits<std::shared_ptr<T>> { - using wrap_category = wrap_type_shared_ptr_tag; +struct input<const std::unique_ptr<T>&> { + using type = ConstRefSmartPtrInput<T, default_unique_ptr>; }; template <typename T> -struct wrap_type_traits<std::unique_ptr<T>> { - using wrap_category = wrap_type_unique_ptr_tag; +struct input<const std::vector<std::shared_ptr<T>>&> { + using type = ConstRefVectorSmartPtrInput<T, std::shared_ptr>; }; -} // namespace traits +} // namespace r +} // namespace arrow -namespace internal { +namespace cpp11 { template <typename T> -inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_shared_ptr_tag); +using enable_if_shared_ptr = typename std::enable_if< + std::is_same<std::shared_ptr<typename T::element_type>, T>::value, T>::type; template <typename T> -inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_unique_ptr_tag); +enable_if_shared_ptr<T> as_cpp(SEXP from) { + return arrow::r::ConstRefSmartPtrInput<typename T::element_type, std::shared_ptr>(from); +} -} // namespace internal -} // namespace Rcpp +template <typename T> +SEXP as_sexp(const std::shared_ptr<T>& ptr) { + return Rcpp::XPtr<std::shared_ptr<T>>(new std::shared_ptr<T>(ptr)); +} -#include <Rcpp.h> +template <typename T> +cpp11::list as_sexp(const std::vector<std::shared_ptr<T>>& vec) { + R_xlen_t n = vec.size(); + SEXP res = PROTECT(Rf_allocVector(VECSXP, n)); + for (R_xlen_t i = 0; i < n; i++) { + SET_VECTOR_ELT(res, i, as_sexp(vec[i])); + } + UNPROTECT(1); + return res; +} -namespace Rcpp { -namespace internal { +template <typename E, typename std::enable_if<std::is_enum<E>::value>::type* = nullptr> +SEXP as_sexp(E e) { + return as_sexp(static_cast<int>(e)); +} -template <typename T> -inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_shared_ptr_tag) { - return Rcpp::XPtr<std::shared_ptr<typename T::element_type>>( - new std::shared_ptr<typename T::element_type>(x)); +} // namespace cpp11 + +namespace arrow { +namespace r { + +template <typename T, typename r_vec, typename Lambda> +r_vec to_r_vector(const std::vector<std::shared_ptr<T>>& x, Lambda lambda) { + auto n = x.size(); + r_vec out(n); + for (int i = 0; i < n; i++) { + out[i] = lambda(x[i]); + } + return out; } -template <typename T> -inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_unique_ptr_tag) { - return Rcpp::XPtr<std::unique_ptr<typename T::element_type>>( - new std::unique_ptr<typename T::element_type>(const_cast<T&>(x).release())); +template <typename T, typename Lambda> +cpp11::writable::strings to_r_strings(const std::vector<std::shared_ptr<T>>& x, + Lambda lambda) { + return to_r_vector<T, cpp11::writable::strings, Lambda>(x, lambda); } -} // namespace internal +template <typename T> +cpp11::writable::list to_r_list(const std::vector<std::shared_ptr<T>>& x) { + auto lambda = [](const std::shared_ptr<T>& t) { return cpp11::as_sexp(t); }; + return to_r_vector<T, cpp11::writable::list, decltype(lambda)>(x, lambda); +} Review comment: ```suggestion template <typename T> cpp11::writable::list to_r_list(const std::vector<std::shared_ptr<T>>& x) { auto as_sexp = [](const std::shared_ptr<T>& t) { return cpp11::as_sexp(t); }; return to_r_vector<cpp11::writable::list>(x, as_sexp); } ``` ########## File path: r/src/arrow_rcpp.h ########## @@ -63,124 +68,222 @@ struct ns { static SEXP arrow; }; +class Index { + public: + // TODO: uncomment when this is merged in cpp11: https://github.com/r-lib/cpp11/pull/58 + /*explicit*/ Index(SEXP x) : index_(validate_index(x)) {} // NOLINT runtime/explicit + + inline operator R_xlen_t() const { return index_; } + + private: + R_xlen_t index_; + + static R_xlen_t validate_index(SEXP x) { + if (XLENGTH(x) == 1) { + switch (TYPEOF(x)) { + case INTSXP: + return INTEGER_ELT(x, 0); + case REALSXP: + if (cpp11::is_convertable_without_loss_to_integer(REAL_ELT(x, 0))) + return REAL_ELT(x, 0); + case LGLSXP: + return LOGICAL_ELT(x, 0); + default: + break; + } + } + + cpp11::stop("Expected single integer value"); + return 0; + } +}; + } // namespace r } // namespace arrow namespace Rcpp { -namespace internal { +using NumericVector_ = Rcpp::Vector<REALSXP, Rcpp::NoProtectStorage>; +using IntegerVector_ = Rcpp::Vector<INTSXP, Rcpp::NoProtectStorage>; +using LogicalVector_ = Rcpp::Vector<LGLSXP, Rcpp::NoProtectStorage>; +using StringVector_ = Rcpp::Vector<STRSXP, Rcpp::NoProtectStorage>; +using CharacterVector_ = StringVector_; +using RawVector_ = Rcpp::Vector<RAWSXP, Rcpp::NoProtectStorage>; +using List_ = Rcpp::Vector<VECSXP, Rcpp::NoProtectStorage>; +} // namespace Rcpp + +namespace cpp11 { + +template <typename E> +typename std::enable_if<std::is_enum<E>::value, E>::type as_cpp(SEXP from) { + return E(cpp11::as_cpp<int>(from)); +} + +} // namespace cpp11 + +namespace arrow { +namespace r { template <typename Pointer> -Pointer r6_to_smart_pointer(SEXP self) { +Pointer r6_to_pointer(SEXP self) { return reinterpret_cast<Pointer>( R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp))); } -} // namespace internal - -template <typename T> -class ConstReferenceSmartPtrInputParameter { +template <typename T, template <class> class SmartPtr> +class ConstRefSmartPtrInput { public: - using const_reference = const T&; + using const_reference = const SmartPtr<T>&; - explicit ConstReferenceSmartPtrInputParameter(SEXP self) - : ptr(internal::r6_to_smart_pointer<const T*>(self)) {} + explicit ConstRefSmartPtrInput(SEXP self) + : ptr(r6_to_pointer<const SmartPtr<T>*>(self)) {} inline operator const_reference() { return *ptr; } private: - const T* ptr; + // this class host + const SmartPtr<T>* ptr; }; -template <typename T> -class ConstReferenceVectorSmartPtrInputParameter { +template <typename T, template <class> class SmartPtr> +class ConstRefVectorSmartPtrInput { public: - using const_reference = const std::vector<T>&; + using const_reference = const std::vector<SmartPtr<T>>&; - explicit ConstReferenceVectorSmartPtrInputParameter(SEXP self) : vec() { + explicit ConstRefVectorSmartPtrInput(SEXP self) : vec() { R_xlen_t n = XLENGTH(self); for (R_xlen_t i = 0; i < n; i++) { - vec.push_back(*internal::r6_to_smart_pointer<const T*>(VECTOR_ELT(self, i))); + vec.push_back(*r6_to_pointer<const SmartPtr<T>*>(VECTOR_ELT(self, i))); } } inline operator const_reference() { return vec; } private: - std::vector<T> vec; + std::vector<SmartPtr<T>> vec; +}; + +template <typename T> +class default_input { + public: + explicit default_input(SEXP from) : from_(from) {} + + operator T() const { return cpp11::as_cpp<T>(from_); } + + private: + SEXP from_; }; -namespace traits { +template <typename T> +class const_reference_input { + public: + explicit const_reference_input(SEXP from) : obj_(cpp11::as_cpp<T>(from)) {} + + using const_reference = const T&; + operator const_reference() const { return obj_; } + + private: + T obj_; +}; template <typename T> -struct input_parameter<const std::shared_ptr<T>&> { - typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<T>> type; +struct input { + using type = default_input<T>; }; template <typename T> -struct input_parameter<const std::unique_ptr<T>&> { - typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::unique_ptr<T>> type; +struct input<const T&> { + using type = const_reference_input<typename std::decay<T>::type>; }; template <typename T> -struct input_parameter<const std::vector<std::shared_ptr<T>>&> { - typedef typename Rcpp::ConstReferenceVectorSmartPtrInputParameter<std::shared_ptr<T>> - type; +struct input<const std::shared_ptr<T>&> { + using type = ConstRefSmartPtrInput<T, std::shared_ptr>; }; -struct wrap_type_shared_ptr_tag {}; -struct wrap_type_unique_ptr_tag {}; +template <typename T> +using default_unique_ptr = std::unique_ptr<T, std::default_delete<T>>; template <typename T> -struct wrap_type_traits<std::shared_ptr<T>> { - using wrap_category = wrap_type_shared_ptr_tag; +struct input<const std::unique_ptr<T>&> { + using type = ConstRefSmartPtrInput<T, default_unique_ptr>; }; template <typename T> -struct wrap_type_traits<std::unique_ptr<T>> { - using wrap_category = wrap_type_unique_ptr_tag; +struct input<const std::vector<std::shared_ptr<T>>&> { + using type = ConstRefVectorSmartPtrInput<T, std::shared_ptr>; }; -} // namespace traits +} // namespace r +} // namespace arrow -namespace internal { +namespace cpp11 { template <typename T> -inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_shared_ptr_tag); +using enable_if_shared_ptr = typename std::enable_if< + std::is_same<std::shared_ptr<typename T::element_type>, T>::value, T>::type; template <typename T> -inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_unique_ptr_tag); +enable_if_shared_ptr<T> as_cpp(SEXP from) { + return arrow::r::ConstRefSmartPtrInput<typename T::element_type, std::shared_ptr>(from); +} -} // namespace internal -} // namespace Rcpp +template <typename T> +SEXP as_sexp(const std::shared_ptr<T>& ptr) { + return Rcpp::XPtr<std::shared_ptr<T>>(new std::shared_ptr<T>(ptr)); +} -#include <Rcpp.h> +template <typename T> +cpp11::list as_sexp(const std::vector<std::shared_ptr<T>>& vec) { Review comment: would be possible/useful to reuse `to_r_list` in here? ########## File path: r/src/buffer.cpp ########## @@ -51,8 +51,7 @@ std::shared_ptr<arrow::Buffer> r___RBuffer__initialize(SEXP x) { case CPLXSXP: return std::make_shared<arrow::r::RBuffer<CPLXSXP>>(x); default: - Rcpp::stop( - tfm::format("R object of type %s not supported", Rf_type2char(TYPEOF(x)))); + cpp11::stop("R object of type <", Rf_type2char(TYPEOF(x)), "> not supported"); Review comment: ```suggestion cpp11::stop("R object of type <%s> not supported", Rf_type2char(TYPEOF(x))); ``` ########## File path: r/src/arrow_rcpp.h ########## @@ -63,124 +68,222 @@ struct ns { static SEXP arrow; }; +class Index { + public: + // TODO: uncomment when this is merged in cpp11: https://github.com/r-lib/cpp11/pull/58 + /*explicit*/ Index(SEXP x) : index_(validate_index(x)) {} // NOLINT runtime/explicit + + inline operator R_xlen_t() const { return index_; } + + private: + R_xlen_t index_; + + static R_xlen_t validate_index(SEXP x) { + if (XLENGTH(x) == 1) { + switch (TYPEOF(x)) { + case INTSXP: + return INTEGER_ELT(x, 0); + case REALSXP: + if (cpp11::is_convertable_without_loss_to_integer(REAL_ELT(x, 0))) + return REAL_ELT(x, 0); + case LGLSXP: + return LOGICAL_ELT(x, 0); + default: + break; + } + } + + cpp11::stop("Expected single integer value"); + return 0; + } +}; + } // namespace r } // namespace arrow namespace Rcpp { -namespace internal { +using NumericVector_ = Rcpp::Vector<REALSXP, Rcpp::NoProtectStorage>; +using IntegerVector_ = Rcpp::Vector<INTSXP, Rcpp::NoProtectStorage>; +using LogicalVector_ = Rcpp::Vector<LGLSXP, Rcpp::NoProtectStorage>; +using StringVector_ = Rcpp::Vector<STRSXP, Rcpp::NoProtectStorage>; +using CharacterVector_ = StringVector_; +using RawVector_ = Rcpp::Vector<RAWSXP, Rcpp::NoProtectStorage>; +using List_ = Rcpp::Vector<VECSXP, Rcpp::NoProtectStorage>; +} // namespace Rcpp + +namespace cpp11 { + +template <typename E> +typename std::enable_if<std::is_enum<E>::value, E>::type as_cpp(SEXP from) { + return E(cpp11::as_cpp<int>(from)); +} + +} // namespace cpp11 + +namespace arrow { +namespace r { template <typename Pointer> -Pointer r6_to_smart_pointer(SEXP self) { +Pointer r6_to_pointer(SEXP self) { return reinterpret_cast<Pointer>( R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp))); } -} // namespace internal - -template <typename T> -class ConstReferenceSmartPtrInputParameter { +template <typename T, template <class> class SmartPtr> +class ConstRefSmartPtrInput { public: - using const_reference = const T&; + using const_reference = const SmartPtr<T>&; - explicit ConstReferenceSmartPtrInputParameter(SEXP self) - : ptr(internal::r6_to_smart_pointer<const T*>(self)) {} + explicit ConstRefSmartPtrInput(SEXP self) + : ptr(r6_to_pointer<const SmartPtr<T>*>(self)) {} inline operator const_reference() { return *ptr; } private: - const T* ptr; + // this class host + const SmartPtr<T>* ptr; }; -template <typename T> -class ConstReferenceVectorSmartPtrInputParameter { +template <typename T, template <class> class SmartPtr> +class ConstRefVectorSmartPtrInput { public: - using const_reference = const std::vector<T>&; + using const_reference = const std::vector<SmartPtr<T>>&; - explicit ConstReferenceVectorSmartPtrInputParameter(SEXP self) : vec() { + explicit ConstRefVectorSmartPtrInput(SEXP self) : vec() { R_xlen_t n = XLENGTH(self); for (R_xlen_t i = 0; i < n; i++) { - vec.push_back(*internal::r6_to_smart_pointer<const T*>(VECTOR_ELT(self, i))); + vec.push_back(*r6_to_pointer<const SmartPtr<T>*>(VECTOR_ELT(self, i))); } } inline operator const_reference() { return vec; } private: - std::vector<T> vec; + std::vector<SmartPtr<T>> vec; +}; + +template <typename T> +class default_input { + public: + explicit default_input(SEXP from) : from_(from) {} + + operator T() const { return cpp11::as_cpp<T>(from_); } + + private: + SEXP from_; }; -namespace traits { +template <typename T> +class const_reference_input { + public: + explicit const_reference_input(SEXP from) : obj_(cpp11::as_cpp<T>(from)) {} + + using const_reference = const T&; + operator const_reference() const { return obj_; } + + private: + T obj_; +}; template <typename T> -struct input_parameter<const std::shared_ptr<T>&> { - typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<T>> type; +struct input { + using type = default_input<T>; }; template <typename T> -struct input_parameter<const std::unique_ptr<T>&> { - typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::unique_ptr<T>> type; +struct input<const T&> { + using type = const_reference_input<typename std::decay<T>::type>; }; template <typename T> -struct input_parameter<const std::vector<std::shared_ptr<T>>&> { - typedef typename Rcpp::ConstReferenceVectorSmartPtrInputParameter<std::shared_ptr<T>> - type; +struct input<const std::shared_ptr<T>&> { + using type = ConstRefSmartPtrInput<T, std::shared_ptr>; }; -struct wrap_type_shared_ptr_tag {}; -struct wrap_type_unique_ptr_tag {}; +template <typename T> +using default_unique_ptr = std::unique_ptr<T, std::default_delete<T>>; template <typename T> -struct wrap_type_traits<std::shared_ptr<T>> { - using wrap_category = wrap_type_shared_ptr_tag; +struct input<const std::unique_ptr<T>&> { + using type = ConstRefSmartPtrInput<T, default_unique_ptr>; }; template <typename T> -struct wrap_type_traits<std::unique_ptr<T>> { - using wrap_category = wrap_type_unique_ptr_tag; +struct input<const std::vector<std::shared_ptr<T>>&> { + using type = ConstRefVectorSmartPtrInput<T, std::shared_ptr>; }; -} // namespace traits +} // namespace r +} // namespace arrow -namespace internal { +namespace cpp11 { template <typename T> -inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_shared_ptr_tag); +using enable_if_shared_ptr = typename std::enable_if< + std::is_same<std::shared_ptr<typename T::element_type>, T>::value, T>::type; template <typename T> -inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_unique_ptr_tag); +enable_if_shared_ptr<T> as_cpp(SEXP from) { + return arrow::r::ConstRefSmartPtrInput<typename T::element_type, std::shared_ptr>(from); +} -} // namespace internal -} // namespace Rcpp +template <typename T> +SEXP as_sexp(const std::shared_ptr<T>& ptr) { + return Rcpp::XPtr<std::shared_ptr<T>>(new std::shared_ptr<T>(ptr)); +} -#include <Rcpp.h> +template <typename T> +cpp11::list as_sexp(const std::vector<std::shared_ptr<T>>& vec) { + R_xlen_t n = vec.size(); + SEXP res = PROTECT(Rf_allocVector(VECSXP, n)); + for (R_xlen_t i = 0; i < n; i++) { + SET_VECTOR_ELT(res, i, as_sexp(vec[i])); + } + UNPROTECT(1); + return res; +} -namespace Rcpp { -namespace internal { +template <typename E, typename std::enable_if<std::is_enum<E>::value>::type* = nullptr> +SEXP as_sexp(E e) { + return as_sexp(static_cast<int>(e)); +} -template <typename T> -inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_shared_ptr_tag) { - return Rcpp::XPtr<std::shared_ptr<typename T::element_type>>( - new std::shared_ptr<typename T::element_type>(x)); +} // namespace cpp11 + +namespace arrow { +namespace r { + +template <typename T, typename r_vec, typename Lambda> +r_vec to_r_vector(const std::vector<std::shared_ptr<T>>& x, Lambda lambda) { + auto n = x.size(); + r_vec out(n); + for (int i = 0; i < n; i++) { + out[i] = lambda(x[i]); + } + return out; } -template <typename T> -inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_unique_ptr_tag) { - return Rcpp::XPtr<std::unique_ptr<typename T::element_type>>( - new std::unique_ptr<typename T::element_type>(const_cast<T&>(x).release())); +template <typename T, typename Lambda> +cpp11::writable::strings to_r_strings(const std::vector<std::shared_ptr<T>>& x, + Lambda lambda) { + return to_r_vector<T, cpp11::writable::strings, Lambda>(x, lambda); } -} // namespace internal +template <typename T> +cpp11::writable::list to_r_list(const std::vector<std::shared_ptr<T>>& x) { + auto lambda = [](const std::shared_ptr<T>& t) { return cpp11::as_sexp(t); }; + return to_r_vector<T, cpp11::writable::list, decltype(lambda)>(x, lambda); +} -} // namespace Rcpp +template <typename T, typename Lambda> +cpp11::writable::list to_r_list(const std::vector<std::shared_ptr<T>>& x, Lambda lambda) { + auto lambda1 = [lambda](const std::shared_ptr<T>& t) { + return cpp11::as_sexp(lambda(t)); + }; + return to_r_vector<T, cpp11::writable::list, decltype(lambda1)>(x, lambda1); +} Review comment: The declared lambda does not escape this scope, so capture by reference is acceptable ```suggestion template <typename T, typename ToListElement> cpp11::writable::list to_r_list(const std::vector<std::shared_ptr<T>>& x, ToListElement&& to_element) { auto as_sexp = [&](const std::shared_ptr<T>& t) { return cpp11::as_sexp(to_element(t)); }; return to_r_vector<cpp11::writable::list>(x, as_sexp); } ``` ########## File path: r/src/arrow_rcpp.h ########## @@ -63,124 +68,222 @@ struct ns { static SEXP arrow; }; +class Index { + public: + // TODO: uncomment when this is merged in cpp11: https://github.com/r-lib/cpp11/pull/58 + /*explicit*/ Index(SEXP x) : index_(validate_index(x)) {} // NOLINT runtime/explicit + + inline operator R_xlen_t() const { return index_; } + + private: + R_xlen_t index_; + + static R_xlen_t validate_index(SEXP x) { + if (XLENGTH(x) == 1) { + switch (TYPEOF(x)) { + case INTSXP: + return INTEGER_ELT(x, 0); + case REALSXP: + if (cpp11::is_convertable_without_loss_to_integer(REAL_ELT(x, 0))) + return REAL_ELT(x, 0); + case LGLSXP: + return LOGICAL_ELT(x, 0); + default: + break; + } + } + + cpp11::stop("Expected single integer value"); + return 0; + } +}; + } // namespace r } // namespace arrow namespace Rcpp { -namespace internal { +using NumericVector_ = Rcpp::Vector<REALSXP, Rcpp::NoProtectStorage>; +using IntegerVector_ = Rcpp::Vector<INTSXP, Rcpp::NoProtectStorage>; +using LogicalVector_ = Rcpp::Vector<LGLSXP, Rcpp::NoProtectStorage>; +using StringVector_ = Rcpp::Vector<STRSXP, Rcpp::NoProtectStorage>; +using CharacterVector_ = StringVector_; +using RawVector_ = Rcpp::Vector<RAWSXP, Rcpp::NoProtectStorage>; +using List_ = Rcpp::Vector<VECSXP, Rcpp::NoProtectStorage>; +} // namespace Rcpp + +namespace cpp11 { + +template <typename E> +typename std::enable_if<std::is_enum<E>::value, E>::type as_cpp(SEXP from) { + return E(cpp11::as_cpp<int>(from)); +} + +} // namespace cpp11 + +namespace arrow { +namespace r { template <typename Pointer> -Pointer r6_to_smart_pointer(SEXP self) { +Pointer r6_to_pointer(SEXP self) { return reinterpret_cast<Pointer>( R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp))); } -} // namespace internal - -template <typename T> -class ConstReferenceSmartPtrInputParameter { +template <typename T, template <class> class SmartPtr> +class ConstRefSmartPtrInput { public: - using const_reference = const T&; + using const_reference = const SmartPtr<T>&; - explicit ConstReferenceSmartPtrInputParameter(SEXP self) - : ptr(internal::r6_to_smart_pointer<const T*>(self)) {} + explicit ConstRefSmartPtrInput(SEXP self) + : ptr(r6_to_pointer<const SmartPtr<T>*>(self)) {} inline operator const_reference() { return *ptr; } private: - const T* ptr; + // this class host + const SmartPtr<T>* ptr; }; -template <typename T> -class ConstReferenceVectorSmartPtrInputParameter { +template <typename T, template <class> class SmartPtr> +class ConstRefVectorSmartPtrInput { public: - using const_reference = const std::vector<T>&; + using const_reference = const std::vector<SmartPtr<T>>&; - explicit ConstReferenceVectorSmartPtrInputParameter(SEXP self) : vec() { + explicit ConstRefVectorSmartPtrInput(SEXP self) : vec() { R_xlen_t n = XLENGTH(self); for (R_xlen_t i = 0; i < n; i++) { - vec.push_back(*internal::r6_to_smart_pointer<const T*>(VECTOR_ELT(self, i))); + vec.push_back(*r6_to_pointer<const SmartPtr<T>*>(VECTOR_ELT(self, i))); } } inline operator const_reference() { return vec; } private: - std::vector<T> vec; + std::vector<SmartPtr<T>> vec; +}; + +template <typename T> +class default_input { + public: + explicit default_input(SEXP from) : from_(from) {} + + operator T() const { return cpp11::as_cpp<T>(from_); } + + private: + SEXP from_; }; -namespace traits { +template <typename T> +class const_reference_input { + public: + explicit const_reference_input(SEXP from) : obj_(cpp11::as_cpp<T>(from)) {} + + using const_reference = const T&; + operator const_reference() const { return obj_; } + + private: + T obj_; +}; template <typename T> -struct input_parameter<const std::shared_ptr<T>&> { - typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<T>> type; +struct input { + using type = default_input<T>; }; template <typename T> -struct input_parameter<const std::unique_ptr<T>&> { - typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::unique_ptr<T>> type; +struct input<const T&> { + using type = const_reference_input<typename std::decay<T>::type>; }; template <typename T> -struct input_parameter<const std::vector<std::shared_ptr<T>>&> { - typedef typename Rcpp::ConstReferenceVectorSmartPtrInputParameter<std::shared_ptr<T>> - type; +struct input<const std::shared_ptr<T>&> { + using type = ConstRefSmartPtrInput<T, std::shared_ptr>; }; -struct wrap_type_shared_ptr_tag {}; -struct wrap_type_unique_ptr_tag {}; +template <typename T> +using default_unique_ptr = std::unique_ptr<T, std::default_delete<T>>; template <typename T> -struct wrap_type_traits<std::shared_ptr<T>> { - using wrap_category = wrap_type_shared_ptr_tag; +struct input<const std::unique_ptr<T>&> { + using type = ConstRefSmartPtrInput<T, default_unique_ptr>; }; template <typename T> -struct wrap_type_traits<std::unique_ptr<T>> { - using wrap_category = wrap_type_unique_ptr_tag; +struct input<const std::vector<std::shared_ptr<T>>&> { + using type = ConstRefVectorSmartPtrInput<T, std::shared_ptr>; }; -} // namespace traits +} // namespace r +} // namespace arrow -namespace internal { +namespace cpp11 { template <typename T> -inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_shared_ptr_tag); +using enable_if_shared_ptr = typename std::enable_if< + std::is_same<std::shared_ptr<typename T::element_type>, T>::value, T>::type; template <typename T> -inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_unique_ptr_tag); +enable_if_shared_ptr<T> as_cpp(SEXP from) { + return arrow::r::ConstRefSmartPtrInput<typename T::element_type, std::shared_ptr>(from); +} -} // namespace internal -} // namespace Rcpp +template <typename T> +SEXP as_sexp(const std::shared_ptr<T>& ptr) { + return Rcpp::XPtr<std::shared_ptr<T>>(new std::shared_ptr<T>(ptr)); +} -#include <Rcpp.h> +template <typename T> +cpp11::list as_sexp(const std::vector<std::shared_ptr<T>>& vec) { + R_xlen_t n = vec.size(); + SEXP res = PROTECT(Rf_allocVector(VECSXP, n)); + for (R_xlen_t i = 0; i < n; i++) { + SET_VECTOR_ELT(res, i, as_sexp(vec[i])); + } + UNPROTECT(1); + return res; +} -namespace Rcpp { -namespace internal { +template <typename E, typename std::enable_if<std::is_enum<E>::value>::type* = nullptr> +SEXP as_sexp(E e) { + return as_sexp(static_cast<int>(e)); +} -template <typename T> -inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_shared_ptr_tag) { - return Rcpp::XPtr<std::shared_ptr<typename T::element_type>>( - new std::shared_ptr<typename T::element_type>(x)); +} // namespace cpp11 + +namespace arrow { +namespace r { + +template <typename T, typename r_vec, typename Lambda> +r_vec to_r_vector(const std::vector<std::shared_ptr<T>>& x, Lambda lambda) { + auto n = x.size(); + r_vec out(n); + for (int i = 0; i < n; i++) { + out[i] = lambda(x[i]); + } + return out; } -template <typename T> -inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_unique_ptr_tag) { - return Rcpp::XPtr<std::unique_ptr<typename T::element_type>>( - new std::unique_ptr<typename T::element_type>(const_cast<T&>(x).release())); +template <typename T, typename Lambda> +cpp11::writable::strings to_r_strings(const std::vector<std::shared_ptr<T>>& x, + Lambda lambda) { + return to_r_vector<T, cpp11::writable::strings, Lambda>(x, lambda); } Review comment: ```suggestion template <typename T, typename ToString> cpp11::writable::strings to_r_strings(const std::vector<std::shared_ptr<T>>& x, ToString&& to_string) { return to_r_vector<cpp11::writable::strings>(x, std::forward<ToString>(to_string)); } ``` ---------------------------------------------------------------- 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: us...@infra.apache.org