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/07/22 15:25:22 UTC
[GitHub] [arrow] romainfrancois opened a new pull request #7819: [R]: Switch from Rcpp to cpp11
romainfrancois opened a new pull request #7819:
URL: https://github.com/apache/arrow/pull/7819
This is very much work in progress. I've started by tackling code generation, as we cannot just use `cpp11::cpp_register()` as is, because e.g. of the `#if defined(ARROW_R_WITH_ARROW)` thing.
Once all code generation goes through cpp11 instead of Rcpp, then I'll remove uses of Rcpp classes, and then eventually we can remove the dependency.
----------------------------------------------------------------
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] romainfrancois commented on pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-676369629
I'm trying to debug what's wrong with the python tests that I previously silenced in this PR:
That looks fine:
``` r
library(reticulate)
#> Warning: package 'reticulate' was built under R version 4.0.2
pa <- reticulate::import("pyarrow")
py <- pa$array(c(1, 2, 3))
str(py)
#> [
#> 1,
#> 2,
#> 3
#> ]
class(py)
#> [1] "pyarrow.lib.DoubleArray" "pyarrow.lib.FloatingPointArray"
#> [3] "pyarrow.lib.NumericArray" "pyarrow.lib.Array"
#> [5] "pyarrow.lib._PandasConvertible" "python.builtin.object"
```
But then if I also load `arrow` I'm getting a segfault on `pa$array()` call:
```r
library(reticulate)
library(arrow)
pa <- reticulate::import("pyarrow")
py <- pa$array(c(1, 2, 3))
```
----------------------------------------------------------------
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] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r460795173
##########
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:
oh yeah that's right, 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] romainfrancois commented on pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-676374381
The error seems to happen on this call:
```r
x$`_export_to_c`(array_ptr, schema_ptr)
```
----------------------------------------------------------------
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] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r471335906
##########
File path: r/src/arrow_cpp11.h
##########
@@ -0,0 +1,243 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <limits>
+#include <memory>
+#include <utility>
+#include <vector>
+#undef Free
+
+namespace cpp11 {
+
+template <typename T>
+SEXP as_sexp(const std::shared_ptr<T>& ptr);
+
+template <typename T>
+SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec);
+
+template <typename E, typename std::enable_if<std::is_enum<E>::value>::type* = nullptr>
+SEXP as_sexp(E e);
+
Review comment:
Definitely.
----------------------------------------------------------------
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] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r471349293
##########
File path: r/src/arrow_cpp11.h
##########
@@ -0,0 +1,243 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <limits>
+#include <memory>
+#include <utility>
+#include <vector>
+#undef Free
+
+namespace cpp11 {
+
+template <typename T>
+SEXP as_sexp(const std::shared_ptr<T>& ptr);
+
+template <typename T>
+SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec);
+
+template <typename E, typename std::enable_if<std::is_enum<E>::value>::type* = nullptr>
+SEXP as_sexp(E e);
+
+} // namespace cpp11
+
+#include <cpp11.hpp>
+
+namespace arrow {
+namespace r {
+struct symbols {
+ static SEXP units;
+ static SEXP tzone;
+ static SEXP xp;
+ static SEXP dot_Internal;
+ static SEXP inspect;
+ static SEXP row_names;
+ static SEXP serialize_arrow_r_metadata;
+ static SEXP as_list;
+ static SEXP ptype;
+ static SEXP byte_width;
+ static SEXP list_size;
+};
+
+struct data {
+ static SEXP classes_POSIXct;
+ static SEXP classes_metadata_r;
+ static SEXP classes_vctrs_list_of;
+ static SEXP classes_tbl_df;
+
+ static SEXP classes_arrow_binary;
+ static SEXP classes_arrow_large_binary;
+ static SEXP classes_arrow_fixed_size_binary;
+
+ static SEXP classes_arrow_list;
+ static SEXP classes_arrow_large_list;
+ static SEXP classes_arrow_fixed_size_list;
+
+ static SEXP classes_factor;
+ static SEXP classes_ordered;
+
+ static SEXP names_metadata;
+ static SEXP empty_raw;
+};
+
+struct ns {
+ static SEXP arrow;
+};
+
+template <typename Pointer>
+Pointer r6_to_pointer(SEXP self) {
+ return reinterpret_cast<Pointer>(
+ R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp)));
+}
+
+template <typename T, template <class> class SmartPtr>
+class ConstRefSmartPtrInput {
+ public:
+ using const_reference = const SmartPtr<T>&;
+
+ explicit ConstRefSmartPtrInput(SEXP self)
+ : ptr(r6_to_pointer<const SmartPtr<T>*>(self)) {}
+
+ inline operator const_reference() { return *ptr; }
+
+ private:
+ // this class host
+ const SmartPtr<T>* ptr;
+};
Review comment:
I believe we need to have an R external pointer to a smart (either shared or unique) pointer, and that we cannot directly have an external pointer to the underlying (e.g. Array) object.
##########
File path: r/src/arrow_cpp11.h
##########
@@ -0,0 +1,243 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <limits>
+#include <memory>
+#include <utility>
+#include <vector>
+#undef Free
+
+namespace cpp11 {
+
+template <typename T>
+SEXP as_sexp(const std::shared_ptr<T>& ptr);
+
+template <typename T>
+SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec);
+
+template <typename E, typename std::enable_if<std::is_enum<E>::value>::type* = nullptr>
+SEXP as_sexp(E e);
+
+} // namespace cpp11
+
+#include <cpp11.hpp>
+
+namespace arrow {
+namespace r {
+struct symbols {
+ static SEXP units;
+ static SEXP tzone;
+ static SEXP xp;
+ static SEXP dot_Internal;
+ static SEXP inspect;
+ static SEXP row_names;
+ static SEXP serialize_arrow_r_metadata;
+ static SEXP as_list;
+ static SEXP ptype;
+ static SEXP byte_width;
+ static SEXP list_size;
+};
+
+struct data {
+ static SEXP classes_POSIXct;
+ static SEXP classes_metadata_r;
+ static SEXP classes_vctrs_list_of;
+ static SEXP classes_tbl_df;
+
+ static SEXP classes_arrow_binary;
+ static SEXP classes_arrow_large_binary;
+ static SEXP classes_arrow_fixed_size_binary;
+
+ static SEXP classes_arrow_list;
+ static SEXP classes_arrow_large_list;
+ static SEXP classes_arrow_fixed_size_list;
+
+ static SEXP classes_factor;
+ static SEXP classes_ordered;
+
+ static SEXP names_metadata;
+ static SEXP empty_raw;
+};
+
+struct ns {
+ static SEXP arrow;
+};
+
+template <typename Pointer>
+Pointer r6_to_pointer(SEXP self) {
+ return reinterpret_cast<Pointer>(
+ R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp)));
+}
+
+template <typename T, template <class> class SmartPtr>
+class ConstRefSmartPtrInput {
+ public:
+ using const_reference = const SmartPtr<T>&;
+
+ explicit ConstRefSmartPtrInput(SEXP self)
+ : ptr(r6_to_pointer<const SmartPtr<T>*>(self)) {}
+
+ inline operator const_reference() { return *ptr; }
+
+ private:
+ // this class host
+ const SmartPtr<T>* ptr;
+};
Review comment:
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] nealrichardson closed pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
nealrichardson closed pull request #7819:
URL: https://github.com/apache/arrow/pull/7819
----------------------------------------------------------------
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] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r471335186
##########
File path: r/src/array_from_vector.cpp
##########
@@ -1064,42 +1063,42 @@ class FixedSizeBinaryVectorConverter : public VectorConverter {
FixedSizeBinaryBuilder* typed_builder_;
};
-template <typename Builder>
+template <typename StringBuilder>
class StringVectorConverter : public VectorConverter {
public:
~StringVectorConverter() {}
Status Init(ArrayBuilder* builder) {
- typed_builder_ = checked_cast<Builder*>(builder);
+ typed_builder_ = checked_cast<StringBuilder*>(builder);
return Status::OK();
}
Status Ingest(SEXP obj) {
ARROW_RETURN_IF(TYPEOF(obj) != STRSXP,
Status::RError("Expecting a character vector"));
- R_xlen_t n = XLENGTH(obj);
- // Reserve enough space before appending
- int64_t size = 0;
- for (R_xlen_t i = 0; i < n; i++) {
- SEXP string_i = STRING_ELT(obj, i);
- if (string_i != NA_STRING) {
- size += XLENGTH(Rf_mkCharCE(Rf_translateCharUTF8(string_i), CE_UTF8));
- }
+ cpp11::strings s(obj);
+ RETURN_NOT_OK(typed_builder_->Reserve(s.size()));
+
+ // note: the total length is calculated without utf8
+ // conversion, so see this more as a hint rather than
+ // the actual total length
+ auto total_length_hint = 0;
+ for (cpp11::r_string si : s) {
+ total_length_hint += (si == NA_STRING) ? 0 : si.size();
Review comment:
Thanks. I wasn't aware of `is_na()`
----------------------------------------------------------------
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] romainfrancois commented on pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-677564052
Looks like this is related to how `cpp11` treats `uintptr_t`:
```cpp
uintptr_t allocate_arrow_schema();
extern "C" SEXP _arrow_allocate_arrow_schema(){
BEGIN_CPP11
return cpp11::as_sexp(allocate_arrow_schema());
END_CPP11
}
```
----------------------------------------------------------------
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] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r465137242
##########
File path: r/R/schema.R
##########
@@ -85,7 +85,6 @@ Schema <- R6Class("Schema",
active = list(
names = function() {
out <- Schema__field_names(self)
- # Hack: Rcpp should set the encoding
Encoding(out) <- "UTF-8"
out
},
Review comment:
is there a test ?
----------------------------------------------------------------
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] nealrichardson commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r472278210
##########
File path: r/tests/testthat/test-buffer.R
##########
@@ -39,6 +39,7 @@ test_that("Buffer can be created from numeric vector", {
})
test_that("Buffer can be created from complex vector", {
+ skip("until cpp11 has a complex vector class")
Review comment:
Is this something we need?
##########
File path: r/src/array_from_vector.cpp
##########
@@ -1064,42 +1062,42 @@ class FixedSizeBinaryVectorConverter : public VectorConverter {
FixedSizeBinaryBuilder* typed_builder_;
};
-template <typename Builder>
+template <typename StringBuilder>
class StringVectorConverter : public VectorConverter {
public:
~StringVectorConverter() {}
Status Init(ArrayBuilder* builder) {
- typed_builder_ = checked_cast<Builder*>(builder);
+ typed_builder_ = checked_cast<StringBuilder*>(builder);
return Status::OK();
}
Status Ingest(SEXP obj) {
ARROW_RETURN_IF(TYPEOF(obj) != STRSXP,
Status::RError("Expecting a character vector"));
- R_xlen_t n = XLENGTH(obj);
- // Reserve enough space before appending
- int64_t size = 0;
- for (R_xlen_t i = 0; i < n; i++) {
- SEXP string_i = STRING_ELT(obj, i);
- if (string_i != NA_STRING) {
- size += XLENGTH(Rf_mkCharCE(Rf_translateCharUTF8(string_i), CE_UTF8));
- }
+ cpp11::strings s(obj);
+ RETURN_NOT_OK(typed_builder_->Reserve(s.size()));
+
+ // note: the total length is calculated without utf8
+ // conversion, so see this more as a hint rather than
+ // the actual total length
Review comment:
Is this true? Is this safe to do? Previously you'd get a truncated string when converting latin1 to utf8 unless you converted before measuring size.
##########
File path: r/DESCRIPTION
##########
@@ -50,6 +49,8 @@ Suggests:
rmarkdown,
testthat,
tibble
+Remotes:
+ r-lib/cpp11#97
Review comment:
When we're ready to merge this, we should vendor cpp11. I made https://issues.apache.org/jira/browse/ARROW-9786 to go back to the released cpp11 before we do the 2.0.0 release (October-ish).
##########
File path: r/tests/testthat/test-python.R
##########
@@ -17,6 +17,8 @@
context("To/from Python")
+skip("for now")
Review comment:
Need to remove this before we merge, of course
##########
File path: r/tests/testthat/test-buffer.R
##########
@@ -39,6 +39,7 @@ test_that("Buffer can be created from numeric vector", {
})
test_that("Buffer can be created from complex vector", {
+ skip("until cpp11 has a complex vector class")
Review comment:
Doing some archaeology, it was added specifically in https://github.com/apache/arrow/commit/13c63bdec9675f08b1631e0514dcf2a890d7e36b (https://issues.apache.org/jira/browse/ARROW-3823). No context in the PR or the JIRA to know if this was just added because it was possible or if it was required for something (that said, no evidence that it was demanded by some real use 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] nealrichardson commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r459523384
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -131,14 +131,14 @@ test_that("Slice() and RangeEquals()", {
expect_true(x$RangeEquals(z, 10, 15, 0))
# Input validation
- expect_error(x$Slice("ten"), class = "Rcpp::not_compatible")
+ expect_error(x$Slice("ten"))
Review comment:
Is there an equivalent class to the exception that cpp11 raises? Should there be?
##########
File path: r/R/array.R
##########
@@ -119,9 +119,9 @@ Array <- R6Class("Array",
},
Slice = function(offset, length = NULL) {
if (is.null(length)) {
- Array$create(Array__Slice1(self, offset))
+ Array$create(Array__Slice1(self, as_index(offset)))
} else {
- Array$create(Array__Slice2(self, offset, length))
+ Array$create(Array__Slice2(self, as_index(offset), length))
Review comment:
If `offset` needs this then so should `length`. Also I expect that you'll want/have to use this in lots of other places.
```suggestion
Array$create(Array__Slice2(self, as_index(offset), as_index(length)))
```
----------------------------------------------------------------
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] kszucs commented on pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-664249577
@romainfrancois please rebase, it has some conflicts with the master.
----------------------------------------------------------------
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] nealrichardson commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r465136902
##########
File path: r/src/arrow_types.h
##########
@@ -17,25 +17,9 @@
#pragma once
-#include "./arrow_rcpp.h"
-
-template <typename T>
-struct NoDelete {
- inline void operator()(T* ptr) {}
-};
-
-namespace Rcpp {
+#include <cpp11/R.hpp>
-template <int RTYPE>
-inline constexpr typename Rcpp::Vector<RTYPE>::stored_type default_value() {
- return Rcpp::Vector<RTYPE>::get_na();
-}
-template <>
-inline constexpr Rbyte default_value<RAWSXP>() {
- return 0;
-}
-
-} // namespace Rcpp
+#include "./arrow_rcpp.h"
Review comment:
Rename this file?
##########
File path: r/src/arrow_rcpp.h
##########
@@ -63,124 +77,212 @@ struct ns {
static SEXP arrow;
};
+class Index {
+ public:
+ // TODO: uncomment when this is merged in cpp11: https://github.com/r-lib/cpp11/pull/58
Review comment:
FTR it is merged
##########
File path: r/README.md
##########
@@ -149,7 +149,7 @@ For any other build/configuration challenges, see the [C++ developer
guide](https://arrow.apache.org/docs/developers/cpp/building.html) and
`vignette("install", package = "arrow")`.
-### Editing Rcpp code
+### Editing C++ code
The `arrow` package uses some customized tools on top of `Rcpp` to
Review comment:
Another Rcpp on this line
----------------------------------------------------------------
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 #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-665141432
----------------------------------------------------------------
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] romainfrancois commented on pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-677661153
I'm assuming there will be follow up, but it looks like the Rcpp -> cpp11 is complete. 🎉
----------------------------------------------------------------
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] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r459550356
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -131,14 +131,14 @@ test_that("Slice() and RangeEquals()", {
expect_true(x$RangeEquals(z, 10, 15, 0))
# Input validation
- expect_error(x$Slice("ten"), class = "Rcpp::not_compatible")
+ expect_error(x$Slice("ten"))
Review comment:
As far as I can see, `cpp11::stop()` calls `Rf_error()` directly without setting specific exception classes. cc @jimhester but it wasn't that useful to have dedicated exception classes.
https://github.com/r-lib/cpp11/blob/master/inst/include/cpp11/protect.hpp#L205
specifically the conversion is refused by `cpp11::stop()` here: https://github.com/r-lib/cpp11/blob/master/inst/include/cpp11/as.hpp#L45 so no additional class.
```cpp
template <typename T>
is_integral<T> as_cpp(SEXP from) {
if (Rf_isInteger(from)) {
if (Rf_xlength(from) == 1) {
return INTEGER_ELT(from, 0);
}
} else if (Rf_isReal(from)) {
if (Rf_xlength(from) == 1) {
double value = REAL_ELT(from, 0);
if (is_convertable_without_loss_to_integer(value)) {
return value;
}
}
}
stop("Expected single integer value");
return 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] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r471349293
##########
File path: r/src/arrow_cpp11.h
##########
@@ -0,0 +1,243 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <limits>
+#include <memory>
+#include <utility>
+#include <vector>
+#undef Free
+
+namespace cpp11 {
+
+template <typename T>
+SEXP as_sexp(const std::shared_ptr<T>& ptr);
+
+template <typename T>
+SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec);
+
+template <typename E, typename std::enable_if<std::is_enum<E>::value>::type* = nullptr>
+SEXP as_sexp(E e);
+
+} // namespace cpp11
+
+#include <cpp11.hpp>
+
+namespace arrow {
+namespace r {
+struct symbols {
+ static SEXP units;
+ static SEXP tzone;
+ static SEXP xp;
+ static SEXP dot_Internal;
+ static SEXP inspect;
+ static SEXP row_names;
+ static SEXP serialize_arrow_r_metadata;
+ static SEXP as_list;
+ static SEXP ptype;
+ static SEXP byte_width;
+ static SEXP list_size;
+};
+
+struct data {
+ static SEXP classes_POSIXct;
+ static SEXP classes_metadata_r;
+ static SEXP classes_vctrs_list_of;
+ static SEXP classes_tbl_df;
+
+ static SEXP classes_arrow_binary;
+ static SEXP classes_arrow_large_binary;
+ static SEXP classes_arrow_fixed_size_binary;
+
+ static SEXP classes_arrow_list;
+ static SEXP classes_arrow_large_list;
+ static SEXP classes_arrow_fixed_size_list;
+
+ static SEXP classes_factor;
+ static SEXP classes_ordered;
+
+ static SEXP names_metadata;
+ static SEXP empty_raw;
+};
+
+struct ns {
+ static SEXP arrow;
+};
+
+template <typename Pointer>
+Pointer r6_to_pointer(SEXP self) {
+ return reinterpret_cast<Pointer>(
+ R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp)));
+}
+
+template <typename T, template <class> class SmartPtr>
+class ConstRefSmartPtrInput {
+ public:
+ using const_reference = const SmartPtr<T>&;
+
+ explicit ConstRefSmartPtrInput(SEXP self)
+ : ptr(r6_to_pointer<const SmartPtr<T>*>(self)) {}
+
+ inline operator const_reference() { return *ptr; }
+
+ private:
+ // this class host
+ const SmartPtr<T>* ptr;
+};
Review comment:
I believe we need to have an R external pointer to a smart (either shared or unique) pointer, and that we cannot directly have an external pointer to the underlying (e.g. Array) object.
----------------------------------------------------------------
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] romainfrancois edited a comment on pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois edited a comment on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-677645712
I think the last commit (https://github.com/apache/arrow/pull/7819/commits/23b179e7d69d6042821a41878b86e37b18cdeb44) restore how things were before switching to `cpp11` wrt how `uintptr_t` goes back and forth.
This uses a `double` as a host for a `uintptr_t` which IIUC is only faithful to 53 bits.
This should really use an external pointer but then I have no idea how python would go about extracting the actual pointer from the external pointer `SEXP`.
This seems to work, at least according to the tests, but I really don't know if the `uintptr_t` hosted in a `double` is fine with ```x$`_export_to_c`() ``` expectations ?
cc @jjallaire @fsaintjacques @nealrichardson
```r
py_to_r.pyarrow.lib.Array <- function(x, ...) {
schema_ptr <- allocate_arrow_schema()
array_ptr <- allocate_arrow_array()
on.exit({
delete_arrow_schema(schema_ptr)
delete_arrow_array(array_ptr)
})
x$`_export_to_c`(array_ptr, schema_ptr)
Array$create(ImportArray(array_ptr, schema_ptr))
}
```
We get a `double` for both `schema_ptr` and `array_ptr` Is that ok ?
----------------------------------------------------------------
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] romainfrancois commented on pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-668608898
I somehow lost the ability to lint locally:
```
r % CLANG_FORMAT=$(which clang-format) ./lint.sh
Traceback (most recent call last):
File "/Users/romainfrancois/git/apache/arrow/r/../cpp/build-support/run_clang_format.py", line 121, in <module>
for filename, diff in pool.starmap(_check_one_file, checker_args):
AttributeError: 'Pool' object has no attribute 'starmap'
```
----------------------------------------------------------------
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] jimhester commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
jimhester commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r462254382
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -131,14 +131,14 @@ test_that("Slice() and RangeEquals()", {
expect_true(x$RangeEquals(z, 10, 15, 0))
# Input validation
- expect_error(x$Slice("ten"), class = "Rcpp::not_compatible")
+ expect_error(x$Slice("ten"))
Review comment:
We could certainly throw a classed exception here if you think it is useful...
----------------------------------------------------------------
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 #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r471664878
##########
File path: r/src/arrow_cpp11.h
##########
@@ -0,0 +1,243 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <limits>
+#include <memory>
+#include <utility>
+#include <vector>
+#undef Free
+
+namespace cpp11 {
+
+template <typename T>
+SEXP as_sexp(const std::shared_ptr<T>& ptr);
+
+template <typename T>
+SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec);
+
+template <typename E, typename std::enable_if<std::is_enum<E>::value>::type* = nullptr>
+SEXP as_sexp(E e);
+
+} // namespace cpp11
+
+#include <cpp11.hpp>
Review comment:
Hmmm, this is a consequence of cpp11 defining the `as_*` conversions in terms of a free function. For now, please add a comment explaining the `#include`'s location. I'll look into a fix for cpp11
----------------------------------------------------------------
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] nealrichardson commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r473080054
##########
File path: r/src/arrow_cpp11.h
##########
@@ -35,8 +36,45 @@ SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec);
// https://github.com/apache/arrow/pull/7819#discussion_r471664878
#include <cpp11.hpp>
+// borrowed from enc package
+// because R does not make these macros available (i.e. from Defn.h)
+#define UTF8_MASK (1 << 3)
+#define ASCII_MASK (1 << 6)
+
+#define IS_ASCII(x) (LEVELS(x) & ASCII_MASK)
+#define IS_UTF8(x) (LEVELS(x) & UTF8_MASK)
+
namespace arrow {
namespace r {
+
+// functions that need to be called from an unwind_protect()
+namespace unsafe {
+
+inline SEXP utf8_r_string(SEXP s) {
+ if (!IS_UTF8(s) && !IS_ASCII(s)) {
Review comment:
Are you sure these utilities are necessary? `translateCharUTF8` also checks IS_UTF8 and IS_ASCII and exits early: https://github.com/wch/r-source/blob/122dcf452ec5eacdd66e165457985bded1af4fee/src/main/sysutils.c#L1085
----------------------------------------------------------------
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] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r460799103
##########
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:
Possibly. I'll have a look
----------------------------------------------------------------
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] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r473009786
##########
File path: r/tests/testthat/test-buffer.R
##########
@@ -39,6 +39,7 @@ test_that("Buffer can be created from numeric vector", {
})
test_that("Buffer can be created from complex vector", {
+ skip("until cpp11 has a complex vector class")
Review comment:
but just in case, I added it, we don't need a full implementation of `cpp11::complexs` for 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] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r473890328
##########
File path: r/src/arrow_cpp11.h
##########
@@ -35,8 +36,45 @@ SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec);
// https://github.com/apache/arrow/pull/7819#discussion_r471664878
#include <cpp11.hpp>
+// borrowed from enc package
+// because R does not make these macros available (i.e. from Defn.h)
+#define UTF8_MASK (1 << 3)
+#define ASCII_MASK (1 << 6)
+
+#define IS_ASCII(x) (LEVELS(x) & ASCII_MASK)
+#define IS_UTF8(x) (LEVELS(x) & UTF8_MASK)
+
namespace arrow {
namespace r {
+
+// functions that need to be called from an unwind_protect()
+namespace unsafe {
+
+inline SEXP utf8_r_string(SEXP s) {
+ if (!IS_UTF8(s) && !IS_ASCII(s)) {
Review comment:
They might not all be necessary, I'm trying to avoid the `Rf_mkChar(Rf_translateCharUTF8())` combo, which would unnecessarily make a SEXP
----------------------------------------------------------------
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] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r459551825
##########
File path: r/R/array.R
##########
@@ -119,9 +119,9 @@ Array <- R6Class("Array",
},
Slice = function(offset, length = NULL) {
if (is.null(length)) {
- Array$create(Array__Slice1(self, offset))
+ Array$create(Array__Slice1(self, as_index(offset)))
} else {
- Array$create(Array__Slice2(self, offset, length))
+ Array$create(Array__Slice2(self, as_index(offset), length))
Review comment:
Yeah, I'm wondering if this could be handled by a C++ class instead. For now, I'm just trying to make it all pass. This is only needed if converting to `int` refuses to deal with `NA` (which is a logical), i.e. https://github.com/r-lib/cpp11/pull/53
----------------------------------------------------------------
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 #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r471335034
##########
File path: r/src/array_from_vector.cpp
##########
@@ -406,9 +403,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 '%s'",
+ dict_type.index_type()->ToString().c_str());
+ return nullptr;
Review comment:
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] romainfrancois commented on pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-677577928
Rcpp makes a `numeric` and `cpp11` makes a `int`:
``` r
library(cpp11)
library(Rcpp)
str(Rcpp::evalCpp("reinterpret_cast<uintptr_t>(new int)"))
#> num 1.41e+14
str(cpp11::cpp_eval("reinterpret_cast<uintptr_t>(new int)"))
#> int -1631515312
```
<sup>Created on 2020-08-20 by the [reprex package](https://reprex.tidyverse.org) (v0.3.0.9001)</sup>
----------------------------------------------------------------
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] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r473891348
##########
File path: r/src/arrow_cpp11.h
##########
@@ -35,8 +36,45 @@ SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec);
// https://github.com/apache/arrow/pull/7819#discussion_r471664878
#include <cpp11.hpp>
+// borrowed from enc package
+// because R does not make these macros available (i.e. from Defn.h)
+#define UTF8_MASK (1 << 3)
+#define ASCII_MASK (1 << 6)
+
+#define IS_ASCII(x) (LEVELS(x) & ASCII_MASK)
+#define IS_UTF8(x) (LEVELS(x) & UTF8_MASK)
+
namespace arrow {
namespace r {
+
+// functions that need to be called from an unwind_protect()
+namespace unsafe {
+
+inline SEXP utf8_r_string(SEXP s) {
+ if (!IS_UTF8(s) && !IS_ASCII(s)) {
Review comment:
This is thin for now, but this might evolve into perhaps something like `cpp11::utf8_strings` class that would eagerly convert at construction time.
----------------------------------------------------------------
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] nealrichardson commented on pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-677730369
@github-actions crossbow submit -g r
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r460031370
##########
File path: r/src/arrow_rcpp.h
##########
@@ -67,120 +71,150 @@ struct ns {
} // 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;
};
-namespace traits {
-
template <typename T>
-struct input_parameter<const std::shared_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<T>> type;
+class default_input {
+ public:
+ default_input(SEXP from) : from_(from) {}
+
+ inline operator T() const { return cpp11::as_cpp<T>(from_); }
+
+ private:
+ SEXP from_;
};
template <typename T>
-struct input_parameter<const std::unique_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::unique_ptr<T>> type;
+class const_reference_input {
+ public:
+ const_reference_input(SEXP from) : obj_(cpp11::as_cpp<T>(from)) {}
+
+ using const_reference = const T&;
+ inline operator const_reference() const { return obj_; }
+
+ private:
+ T obj_;
};
template <typename T>
-struct input_parameter<const std::vector<std::shared_ptr<T>>&> {
- typedef typename Rcpp::ConstReferenceVectorSmartPtrInputParameter<std::shared_ptr<T>>
- type;
+struct input {
+ using type = default_input<T>;
};
-struct wrap_type_shared_ptr_tag {};
-struct wrap_type_unique_ptr_tag {};
-
template <typename T>
-struct wrap_type_traits<std::shared_ptr<T>> {
- using wrap_category = wrap_type_shared_ptr_tag;
+struct input<const T&> {
+ using type = const_reference_input<typename std::decay<T>::type>;
};
template <typename T>
-struct wrap_type_traits<std::unique_ptr<T>> {
- using wrap_category = wrap_type_unique_ptr_tag;
+struct input<const std::shared_ptr<T>&> {
+ using type = ConstRefSmartPtrInput<T, std::shared_ptr>;
};
-} // namespace traits
-
-namespace internal {
+template <typename T>
+using default_unique_ptr = std::unique_ptr<T, std::default_delete<T>>;
template <typename T>
-inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_shared_ptr_tag);
+struct input<const std::unique_ptr<T>&> {
+ using type = ConstRefSmartPtrInput<T, default_unique_ptr>;
+};
template <typename T>
-inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_unique_ptr_tag);
+struct input<const std::vector<std::shared_ptr<T>>&> {
+ using type = ConstRefVectorSmartPtrInput<T, std::shared_ptr>;
+};
-} // namespace internal
-} // namespace Rcpp
+} // namespace r
+} // namespace arrow
-#include <Rcpp.h>
+namespace cpp11 {
-namespace Rcpp {
-namespace internal {
+template <typename T>
+using is_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_shared_ptr_tag) {
- return Rcpp::XPtr<std::shared_ptr<typename T::element_type>>(
- new std::shared_ptr<typename T::element_type>(x));
+is_shared_ptr<T> as_cpp(SEXP from) {
Review comment:
Thanks. I find this clearer indeed. I was following @jimhester lead from https://github.com/r-lib/cpp11/blob/master/inst/include/cpp11/as.hpp
@jimhester would you like a pr to replace some of the `is_*` templates with `enable_if_*` ?
----------------------------------------------------------------
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 #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r470880501
##########
File path: r/src/arrow_cpp11.h
##########
@@ -0,0 +1,243 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <limits>
+#include <memory>
+#include <utility>
+#include <vector>
+#undef Free
+
+namespace cpp11 {
+
+template <typename T>
+SEXP as_sexp(const std::shared_ptr<T>& ptr);
+
+template <typename T>
+SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec);
+
+template <typename E, typename std::enable_if<std::is_enum<E>::value>::type* = nullptr>
+SEXP as_sexp(E e);
+
+} // namespace cpp11
+
+#include <cpp11.hpp>
+
+namespace arrow {
+namespace r {
+struct symbols {
+ static SEXP units;
+ static SEXP tzone;
+ static SEXP xp;
+ static SEXP dot_Internal;
+ static SEXP inspect;
+ static SEXP row_names;
+ static SEXP serialize_arrow_r_metadata;
+ static SEXP as_list;
+ static SEXP ptype;
+ static SEXP byte_width;
+ static SEXP list_size;
+};
+
+struct data {
+ static SEXP classes_POSIXct;
+ static SEXP classes_metadata_r;
+ static SEXP classes_vctrs_list_of;
+ static SEXP classes_tbl_df;
+
+ static SEXP classes_arrow_binary;
+ static SEXP classes_arrow_large_binary;
+ static SEXP classes_arrow_fixed_size_binary;
+
+ static SEXP classes_arrow_list;
+ static SEXP classes_arrow_large_list;
+ static SEXP classes_arrow_fixed_size_list;
+
+ static SEXP classes_factor;
+ static SEXP classes_ordered;
+
+ static SEXP names_metadata;
+ static SEXP empty_raw;
+};
+
+struct ns {
+ static SEXP arrow;
+};
+
+template <typename Pointer>
+Pointer r6_to_pointer(SEXP self) {
+ return reinterpret_cast<Pointer>(
+ R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp)));
+}
+
+template <typename T, template <class> class SmartPtr>
+class ConstRefSmartPtrInput {
+ public:
+ using const_reference = const SmartPtr<T>&;
+
+ explicit ConstRefSmartPtrInput(SEXP self)
+ : ptr(r6_to_pointer<const SmartPtr<T>*>(self)) {}
+
+ inline operator const_reference() { return *ptr; }
+
+ private:
+ // this class host
+ const SmartPtr<T>* ptr;
+};
Review comment:
actually, can we use cpp11::external_pointer to replace this and other things which depend on r6_to_pointer
----------------------------------------------------------------
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 #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-662541583
https://issues.apache.org/jira/browse/ARROW-9405
----------------------------------------------------------------
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] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r472991503
##########
File path: r/src/array_from_vector.cpp
##########
@@ -1064,42 +1062,42 @@ class FixedSizeBinaryVectorConverter : public VectorConverter {
FixedSizeBinaryBuilder* typed_builder_;
};
-template <typename Builder>
+template <typename StringBuilder>
class StringVectorConverter : public VectorConverter {
public:
~StringVectorConverter() {}
Status Init(ArrayBuilder* builder) {
- typed_builder_ = checked_cast<Builder*>(builder);
+ typed_builder_ = checked_cast<StringBuilder*>(builder);
return Status::OK();
}
Status Ingest(SEXP obj) {
ARROW_RETURN_IF(TYPEOF(obj) != STRSXP,
Status::RError("Expecting a character vector"));
- R_xlen_t n = XLENGTH(obj);
- // Reserve enough space before appending
- int64_t size = 0;
- for (R_xlen_t i = 0; i < n; i++) {
- SEXP string_i = STRING_ELT(obj, i);
- if (string_i != NA_STRING) {
- size += XLENGTH(Rf_mkCharCE(Rf_translateCharUTF8(string_i), CE_UTF8));
- }
+ cpp11::strings s(obj);
+ RETURN_NOT_OK(typed_builder_->Reserve(s.size()));
+
+ // note: the total length is calculated without utf8
+ // conversion, so see this more as a hint rather than
+ // the actual total length
Review comment:
I've changed the strategy to use conversion upfront (only when needed) so that we can get a real total size, and then use UnsafeAppend
----------------------------------------------------------------
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] romainfrancois removed a comment on pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois removed a comment on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-668608898
I somehow lost the ability to lint locally:
```
r % CLANG_FORMAT=$(which clang-format) ./lint.sh
Traceback (most recent call last):
File "/Users/romainfrancois/git/apache/arrow/r/../cpp/build-support/run_clang_format.py", line 121, in <module>
for filename, diff in pool.starmap(_check_one_file, checker_args):
AttributeError: 'Pool' object has no attribute 'starmap'
```
----------------------------------------------------------------
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 #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-677731472
Revision: fc6597e85a0d8c1a54b75170a0f29002474605e8
Submitted crossbow builds: [ursa-labs/crossbow @ actions-495](https://github.com/ursa-labs/crossbow/branches/all?query=actions-495)
|Task|Status|
|----|------|
|homebrew-r-autobrew|[](https://travis-ci.org/ursa-labs/crossbow/branches)|
|test-conda-r-4.0|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-495-github-test-conda-r-4.0)|
|test-r-linux-as-cran|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-495-github-test-r-linux-as-cran)|
|test-r-rhub-ubuntu-gcc-release|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-495-azure-test-r-rhub-ubuntu-gcc-release)|
|test-r-rocker-r-base-latest|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-495-azure-test-r-rocker-r-base-latest)|
|test-r-rstudio-r-base-3.6-bionic|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-495-azure-test-r-rstudio-r-base-3.6-bionic)|
|test-r-rstudio-r-base-3.6-centos6|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-495-azure-test-r-rstudio-r-base-3.6-centos6)|
|test-r-rstudio-r-base-3.6-centos8|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-495-azure-test-r-rstudio-r-base-3.6-centos8)|
|test-r-rstudio-r-base-3.6-opensuse15|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-495-azure-test-r-rstudio-r-base-3.6-opensuse15)|
|test-r-rstudio-r-base-3.6-opensuse42|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-495-azure-test-r-rstudio-r-base-3.6-opensuse42)|
|test-ubuntu-18.04-r-sanitizer|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-495-azure-test-ubuntu-18.04-r-sanitizer)|
----------------------------------------------------------------
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] romainfrancois edited a comment on pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois edited a comment on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-676374381
The error seems to happen on this call:
```r
x$`_export_to_c`(array_ptr, schema_ptr)
```
although there seems to be something there:
```r
Browse[2]> x$`_export_to_c`
<built-in method _export_to_c of pyarrow.lib.DoubleArray>
```
----------------------------------------------------------------
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] nealrichardson commented on pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-668666662
@github-actions autotune everything
----------------------------------------------------------------
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 #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r467084647
##########
File path: r/src/array_from_vector.cpp
##########
@@ -406,9 +403,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 '%s'",
+ dict_type.index_type()->ToString().c_str());
+ return nullptr;
Review comment:
There *should* be no need to return here since `cpp11::stop()` is marked `[[noreturn]]`
##########
File path: r/src/array_from_vector.cpp
##########
@@ -1064,42 +1063,42 @@ class FixedSizeBinaryVectorConverter : public VectorConverter {
FixedSizeBinaryBuilder* typed_builder_;
};
-template <typename Builder>
+template <typename StringBuilder>
class StringVectorConverter : public VectorConverter {
public:
~StringVectorConverter() {}
Status Init(ArrayBuilder* builder) {
- typed_builder_ = checked_cast<Builder*>(builder);
+ typed_builder_ = checked_cast<StringBuilder*>(builder);
return Status::OK();
}
Status Ingest(SEXP obj) {
ARROW_RETURN_IF(TYPEOF(obj) != STRSXP,
Status::RError("Expecting a character vector"));
- R_xlen_t n = XLENGTH(obj);
- // Reserve enough space before appending
- int64_t size = 0;
- for (R_xlen_t i = 0; i < n; i++) {
- SEXP string_i = STRING_ELT(obj, i);
- if (string_i != NA_STRING) {
- size += XLENGTH(Rf_mkCharCE(Rf_translateCharUTF8(string_i), CE_UTF8));
- }
+ cpp11::strings s(obj);
+ RETURN_NOT_OK(typed_builder_->Reserve(s.size()));
+
+ // note: the total length is calculated without utf8
+ // conversion, so see this more as a hint rather than
+ // the actual total length
+ auto total_length_hint = 0;
Review comment:
```suggestion
int64_t total_length_hint = 0;
```
##########
File path: r/src/arrow_cpp11.h
##########
@@ -0,0 +1,243 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <limits>
+#include <memory>
+#include <utility>
+#include <vector>
+#undef Free
+
+namespace cpp11 {
+
+template <typename T>
+SEXP as_sexp(const std::shared_ptr<T>& ptr);
+
+template <typename T>
+SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec);
+
+template <typename E, typename std::enable_if<std::is_enum<E>::value>::type* = nullptr>
+SEXP as_sexp(E e);
+
+} // namespace cpp11
+
+#include <cpp11.hpp>
Review comment:
please keep all includes at the top of the file
##########
File path: r/src/arrow_cpp11.h
##########
@@ -0,0 +1,243 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <limits>
+#include <memory>
+#include <utility>
+#include <vector>
+#undef Free
+
+namespace cpp11 {
+
+template <typename T>
+SEXP as_sexp(const std::shared_ptr<T>& ptr);
+
+template <typename T>
+SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec);
+
+template <typename E, typename std::enable_if<std::is_enum<E>::value>::type* = nullptr>
+SEXP as_sexp(E e);
+
+} // namespace cpp11
+
+#include <cpp11.hpp>
+
+namespace arrow {
+namespace r {
+struct symbols {
+ static SEXP units;
+ static SEXP tzone;
+ static SEXP xp;
+ static SEXP dot_Internal;
+ static SEXP inspect;
+ static SEXP row_names;
+ static SEXP serialize_arrow_r_metadata;
+ static SEXP as_list;
+ static SEXP ptype;
+ static SEXP byte_width;
+ static SEXP list_size;
+};
+
+struct data {
+ static SEXP classes_POSIXct;
+ static SEXP classes_metadata_r;
+ static SEXP classes_vctrs_list_of;
+ static SEXP classes_tbl_df;
+
+ static SEXP classes_arrow_binary;
+ static SEXP classes_arrow_large_binary;
+ static SEXP classes_arrow_fixed_size_binary;
+
+ static SEXP classes_arrow_list;
+ static SEXP classes_arrow_large_list;
+ static SEXP classes_arrow_fixed_size_list;
+
+ static SEXP classes_factor;
+ static SEXP classes_ordered;
+
+ static SEXP names_metadata;
+ static SEXP empty_raw;
+};
+
+struct ns {
+ static SEXP arrow;
+};
+
+template <typename Pointer>
+Pointer r6_to_pointer(SEXP self) {
+ return reinterpret_cast<Pointer>(
+ R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp)));
+}
+
+template <typename T, template <class> class SmartPtr>
+class ConstRefSmartPtrInput {
+ public:
+ using const_reference = const SmartPtr<T>&;
+
+ explicit ConstRefSmartPtrInput(SEXP self)
+ : ptr(r6_to_pointer<const SmartPtr<T>*>(self)) {}
+
+ inline operator const_reference() { return *ptr; }
+
+ private:
+ // this class host
+ const SmartPtr<T>* ptr;
+};
+
+template <typename T, template <class> class SmartPtr>
+class ConstRefVectorSmartPtrInput {
+ public:
+ using const_reference = const std::vector<SmartPtr<T>>&;
+
+ explicit ConstRefVectorSmartPtrInput(SEXP self) : vec() {
+ R_xlen_t n = XLENGTH(self);
+ for (R_xlen_t i = 0; i < n; i++) {
+ vec.push_back(*r6_to_pointer<const SmartPtr<T>*>(VECTOR_ELT(self, i)));
+ }
+ }
+
+ inline operator const_reference() { return vec; }
+
+ private:
+ std::vector<SmartPtr<T>> vec;
+};
Review comment:
Similarly:
```suggestion
template <typename T>
class VectorExternalPtrInput {
public:
explicit VectorExternalPtrInput(SEXP self) : vec_(XLENGTH(self)) {
R_xlen_t i = 0;
for (auto& element : vec_) {
element = *r6_to_pointer<const T*>(VECTOR_ELT(self, i++));
}
}
operator const std::vector<T>&() const { return vec_; }
private:
std::vector<T> vec_;
};
```
used as:
```c++
VectorExternalPtrInput<std::shared_ptr<Array>> array_vector{chunks};
```
##########
File path: r/src/arrow_cpp11.h
##########
@@ -0,0 +1,243 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <limits>
+#include <memory>
+#include <utility>
+#include <vector>
+#undef Free
+
+namespace cpp11 {
+
+template <typename T>
+SEXP as_sexp(const std::shared_ptr<T>& ptr);
+
+template <typename T>
+SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec);
+
+template <typename E, typename std::enable_if<std::is_enum<E>::value>::type* = nullptr>
+SEXP as_sexp(E e);
+
Review comment:
```suggestion
```
can this be removed after https://github.com/r-lib/cpp11/pull/65 ?
##########
File path: r/src/arrow_cpp11.h
##########
@@ -0,0 +1,243 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <limits>
+#include <memory>
+#include <utility>
+#include <vector>
+#undef Free
+
+namespace cpp11 {
+
+template <typename T>
+SEXP as_sexp(const std::shared_ptr<T>& ptr);
+
+template <typename T>
+SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec);
+
+template <typename E, typename std::enable_if<std::is_enum<E>::value>::type* = nullptr>
+SEXP as_sexp(E e);
+
+} // namespace cpp11
+
+#include <cpp11.hpp>
+
+namespace arrow {
+namespace r {
+struct symbols {
+ static SEXP units;
+ static SEXP tzone;
+ static SEXP xp;
+ static SEXP dot_Internal;
+ static SEXP inspect;
+ static SEXP row_names;
+ static SEXP serialize_arrow_r_metadata;
+ static SEXP as_list;
+ static SEXP ptype;
+ static SEXP byte_width;
+ static SEXP list_size;
+};
+
+struct data {
+ static SEXP classes_POSIXct;
+ static SEXP classes_metadata_r;
+ static SEXP classes_vctrs_list_of;
+ static SEXP classes_tbl_df;
+
+ static SEXP classes_arrow_binary;
+ static SEXP classes_arrow_large_binary;
+ static SEXP classes_arrow_fixed_size_binary;
+
+ static SEXP classes_arrow_list;
+ static SEXP classes_arrow_large_list;
+ static SEXP classes_arrow_fixed_size_list;
+
+ static SEXP classes_factor;
+ static SEXP classes_ordered;
+
+ static SEXP names_metadata;
+ static SEXP empty_raw;
+};
+
+struct ns {
+ static SEXP arrow;
+};
+
+template <typename Pointer>
+Pointer r6_to_pointer(SEXP self) {
+ return reinterpret_cast<Pointer>(
+ R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp)));
+}
+
+template <typename T, template <class> class SmartPtr>
+class ConstRefSmartPtrInput {
+ public:
+ using const_reference = const SmartPtr<T>&;
+
+ explicit ConstRefSmartPtrInput(SEXP self)
+ : ptr(r6_to_pointer<const SmartPtr<T>*>(self)) {}
+
+ inline operator const_reference() { return *ptr; }
+
+ private:
+ // this class host
+ const SmartPtr<T>* ptr;
+};
Review comment:
I don't think this class needs to be specific to smart pointers:
```suggestion
template <typename T>
class ExternalPtrInput {
public:
explicit ExternalPtrInput(SEXP self)
: ptr_(r6_to_pointer<const T*>(self)) {}
operator const T&() const { return *ptr_; }
private:
// this class host
const T* ptr_;
};
```
Then we can use it like:
```c++
ExternalPtrInput<std::shared_ptr<Array>> array{array_sexp};
```
##########
File path: r/src/array_from_vector.cpp
##########
@@ -1064,42 +1063,42 @@ class FixedSizeBinaryVectorConverter : public VectorConverter {
FixedSizeBinaryBuilder* typed_builder_;
};
-template <typename Builder>
+template <typename StringBuilder>
class StringVectorConverter : public VectorConverter {
public:
~StringVectorConverter() {}
Status Init(ArrayBuilder* builder) {
- typed_builder_ = checked_cast<Builder*>(builder);
+ typed_builder_ = checked_cast<StringBuilder*>(builder);
return Status::OK();
}
Status Ingest(SEXP obj) {
ARROW_RETURN_IF(TYPEOF(obj) != STRSXP,
Status::RError("Expecting a character vector"));
- R_xlen_t n = XLENGTH(obj);
- // Reserve enough space before appending
- int64_t size = 0;
- for (R_xlen_t i = 0; i < n; i++) {
- SEXP string_i = STRING_ELT(obj, i);
- if (string_i != NA_STRING) {
- size += XLENGTH(Rf_mkCharCE(Rf_translateCharUTF8(string_i), CE_UTF8));
- }
+ cpp11::strings s(obj);
+ RETURN_NOT_OK(typed_builder_->Reserve(s.size()));
+
+ // note: the total length is calculated without utf8
+ // conversion, so see this more as a hint rather than
+ // the actual total length
+ auto total_length_hint = 0;
+ for (cpp11::r_string si : s) {
+ total_length_hint += (si == NA_STRING) ? 0 : si.size();
Review comment:
Style question: should we prefer `cpp11` utilities over things like `NA_STRING`?
```suggestion
total_length_hint += cpp11::is_na(si) ? 0 : si.size();
```
##########
File path: r/src/arrow_cpp11.h
##########
@@ -0,0 +1,243 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <limits>
+#include <memory>
+#include <utility>
+#include <vector>
+#undef Free
+
+namespace cpp11 {
+
+template <typename T>
+SEXP as_sexp(const std::shared_ptr<T>& ptr);
+
+template <typename T>
+SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec);
+
+template <typename E, typename std::enable_if<std::is_enum<E>::value>::type* = nullptr>
+SEXP as_sexp(E e);
+
+} // namespace cpp11
+
+#include <cpp11.hpp>
+
+namespace arrow {
+namespace r {
+struct symbols {
+ static SEXP units;
+ static SEXP tzone;
+ static SEXP xp;
+ static SEXP dot_Internal;
+ static SEXP inspect;
+ static SEXP row_names;
+ static SEXP serialize_arrow_r_metadata;
+ static SEXP as_list;
+ static SEXP ptype;
+ static SEXP byte_width;
+ static SEXP list_size;
+};
+
+struct data {
+ static SEXP classes_POSIXct;
+ static SEXP classes_metadata_r;
+ static SEXP classes_vctrs_list_of;
+ static SEXP classes_tbl_df;
+
+ static SEXP classes_arrow_binary;
+ static SEXP classes_arrow_large_binary;
+ static SEXP classes_arrow_fixed_size_binary;
+
+ static SEXP classes_arrow_list;
+ static SEXP classes_arrow_large_list;
+ static SEXP classes_arrow_fixed_size_list;
+
+ static SEXP classes_factor;
+ static SEXP classes_ordered;
+
+ static SEXP names_metadata;
+ static SEXP empty_raw;
+};
+
+struct ns {
+ static SEXP arrow;
+};
+
+template <typename Pointer>
+Pointer r6_to_pointer(SEXP self) {
+ return reinterpret_cast<Pointer>(
+ R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp)));
+}
+
+template <typename T, template <class> class SmartPtr>
+class ConstRefSmartPtrInput {
+ public:
+ using const_reference = const SmartPtr<T>&;
+
+ explicit ConstRefSmartPtrInput(SEXP self)
+ : ptr(r6_to_pointer<const SmartPtr<T>*>(self)) {}
+
+ inline operator const_reference() { return *ptr; }
+
+ private:
+ // this class host
+ const SmartPtr<T>* ptr;
+};
+
+template <typename T, template <class> class SmartPtr>
+class ConstRefVectorSmartPtrInput {
+ public:
+ using const_reference = const std::vector<SmartPtr<T>>&;
+
+ explicit ConstRefVectorSmartPtrInput(SEXP self) : vec() {
+ R_xlen_t n = XLENGTH(self);
+ for (R_xlen_t i = 0; i < n; i++) {
+ vec.push_back(*r6_to_pointer<const SmartPtr<T>*>(VECTOR_ELT(self, i)));
+ }
+ }
+
+ inline operator const_reference() { return vec; }
+
+ private:
+ std::vector<SmartPtr<T>> vec;
+};
+
+template <typename T>
+class default_input {
Review comment:
These are named with snake case, which differs from ConstRefSmartPtrInput. Is that intentional? I'm fine with either convention but we should probably stick with one
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r474038493
##########
File path: r/src/Makevars.in
##########
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
-PKG_CPPFLAGS=@cflags@
+PKG_CPPFLAGS=@cflags@ -I../inst/include/
Review comment:
Is there another way @jimhester ? I've had to add this after `cpp_vendor()`
----------------------------------------------------------------
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] romainfrancois commented on pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-677596450
Rcpp has these :
```cpp
/* long are represented as numeric vectors which allows more precision
to be preserved than int */
template<> struct r_sexptype_traits<long>{ enum{ rtype = REALSXP } ; } ;
template<> struct r_sexptype_traits<unsigned long>{ enum{ rtype = REALSXP } ; } ;
```
so `uintptr_t` aka `unsigned long` (at least locally for me) get converted to a `double`
I believe both are wrong (in terms of what this tries to do), but `cpp11` is wronger.
----------------------------------------------------------------
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] romainfrancois commented on pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-676422266
And I can confirm that this works on master locally:
``` r
library(reticulate)
#> Warning: package 'reticulate' was built under R version 4.0.2
library(arrow)
#>
#> Attaching package: 'arrow'
#> The following object is masked from 'package:utils':
#>
#> timestamp
pa <- reticulate::import("pyarrow")
py <- pa$array(c(1, 2, 3))
py
#> Array
#> <double>
#> [
#> 1,
#> 2,
#> 3
#> ]
```
<sup>Created on 2020-08-19 by the [reprex package](https://reprex.tidyverse.org) (v0.3.0.9001)</sup>
More investigation needed.
----------------------------------------------------------------
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] jimhester commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
jimhester commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r464346839
##########
File path: r/src/arrow_rcpp.h
##########
@@ -67,120 +71,150 @@ struct ns {
} // 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;
};
-namespace traits {
-
template <typename T>
-struct input_parameter<const std::shared_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<T>> type;
+class default_input {
+ public:
+ default_input(SEXP from) : from_(from) {}
+
+ inline operator T() const { return cpp11::as_cpp<T>(from_); }
+
+ private:
+ SEXP from_;
};
template <typename T>
-struct input_parameter<const std::unique_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::unique_ptr<T>> type;
+class const_reference_input {
+ public:
+ const_reference_input(SEXP from) : obj_(cpp11::as_cpp<T>(from)) {}
+
+ using const_reference = const T&;
+ inline operator const_reference() const { return obj_; }
+
+ private:
+ T obj_;
};
template <typename T>
-struct input_parameter<const std::vector<std::shared_ptr<T>>&> {
- typedef typename Rcpp::ConstReferenceVectorSmartPtrInputParameter<std::shared_ptr<T>>
- type;
+struct input {
+ using type = default_input<T>;
};
-struct wrap_type_shared_ptr_tag {};
-struct wrap_type_unique_ptr_tag {};
-
template <typename T>
-struct wrap_type_traits<std::shared_ptr<T>> {
- using wrap_category = wrap_type_shared_ptr_tag;
+struct input<const T&> {
+ using type = const_reference_input<typename std::decay<T>::type>;
};
template <typename T>
-struct wrap_type_traits<std::unique_ptr<T>> {
- using wrap_category = wrap_type_unique_ptr_tag;
+struct input<const std::shared_ptr<T>&> {
+ using type = ConstRefSmartPtrInput<T, std::shared_ptr>;
};
-} // namespace traits
-
-namespace internal {
+template <typename T>
+using default_unique_ptr = std::unique_ptr<T, std::default_delete<T>>;
template <typename T>
-inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_shared_ptr_tag);
+struct input<const std::unique_ptr<T>&> {
+ using type = ConstRefSmartPtrInput<T, default_unique_ptr>;
+};
template <typename T>
-inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_unique_ptr_tag);
+struct input<const std::vector<std::shared_ptr<T>>&> {
+ using type = ConstRefVectorSmartPtrInput<T, std::shared_ptr>;
+};
-} // namespace internal
-} // namespace Rcpp
+} // namespace r
+} // namespace arrow
-#include <Rcpp.h>
+namespace cpp11 {
-namespace Rcpp {
-namespace internal {
+template <typename T>
+using is_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_shared_ptr_tag) {
- return Rcpp::XPtr<std::shared_ptr<typename T::element_type>>(
- new std::shared_ptr<typename T::element_type>(x));
+is_shared_ptr<T> as_cpp(SEXP from) {
Review comment:
Sure, that would be great. The issue for this is https://github.com/r-lib/cpp11/issues/66.
Sorry for the late reply, I didn't see this particular mention until just 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] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r464531038
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -131,14 +131,14 @@ test_that("Slice() and RangeEquals()", {
expect_true(x$RangeEquals(z, 10, 15, 0))
# Input validation
- expect_error(x$Slice("ten"), class = "Rcpp::not_compatible")
+ expect_error(x$Slice("ten"))
Review comment:
They were a bit over designed in Rcpp, I think a wrapper around `rlang::abort()` might help though. Some way to have:
- a class to the R condition
- and then some context data, e.g. the `...` from ?abort
I don't think it makes a lot of sense to have various C++ exception classes
----------------------------------------------------------------
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] nealrichardson commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r465135254
##########
File path: r/R/schema.R
##########
@@ -85,7 +85,6 @@ Schema <- R6Class("Schema",
active = list(
names = function() {
out <- Schema__field_names(self)
- # Hack: Rcpp should set the encoding
Encoding(out) <- "UTF-8"
out
},
Review comment:
If `cpp11` is doing what it claims wrt unicode, you can remove this and just
```r
names = function() Schema__field_names(self),
```
----------------------------------------------------------------
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] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r473966560
##########
File path: r/tests/testthat/test-python.R
##########
@@ -17,6 +17,8 @@
context("To/from Python")
+skip("for now")
Review comment:
✔️
----------------------------------------------------------------
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] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r472790386
##########
File path: r/tests/testthat/test-buffer.R
##########
@@ -39,6 +39,7 @@ test_that("Buffer can be created from numeric vector", {
})
test_that("Buffer can be created from complex vector", {
+ skip("until cpp11 has a complex vector class")
Review comment:
I don't think that we do need it.
----------------------------------------------------------------
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] romainfrancois commented on pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-677552027
Getting these details:
<img width="593" alt="image" src="https://user-images.githubusercontent.com/2625526/90764110-1faddf00-e2e8-11ea-8d1a-35ea762c564c.png">
----------------------------------------------------------------
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] romainfrancois commented on a change in pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r471347426
##########
File path: r/src/arrow_cpp11.h
##########
@@ -0,0 +1,243 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <limits>
+#include <memory>
+#include <utility>
+#include <vector>
+#undef Free
+
+namespace cpp11 {
+
+template <typename T>
+SEXP as_sexp(const std::shared_ptr<T>& ptr);
+
+template <typename T>
+SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec);
+
+template <typename E, typename std::enable_if<std::is_enum<E>::value>::type* = nullptr>
+SEXP as_sexp(E e);
+
+} // namespace cpp11
+
+#include <cpp11.hpp>
Review comment:
The `as_sexp()` just above need to happen before including `cpp11.hpp` otherwise we get something like this:
```
de' -I/usr/local/include -fPIC -Wall -O3 -Wall -Wimplicit-int-float-conversion -c filesystem.cpp -o filesystem.o
In file included from filesystem.cpp:18:
In file included from ././arrow_types.h:22:
In file included from ././arrow_cpp11.h:24:
In file included from /Users/romainfrancois/.R/library/4.0/cpp11/include/cpp11.hpp:7:
In file included from /Users/romainfrancois/.R/library/4.0/cpp11/include/cpp11/data_frame.hpp:12:
In file included from /Users/romainfrancois/.R/library/4.0/cpp11/include/cpp11/list.hpp:7:
/Users/romainfrancois/.R/library/4.0/cpp11/include/cpp11/named_arg.hpp:21:14: error: call to function 'as_sexp' that is neither visible in the template definition nor found by argument-dependent lookup
value_ = as_sexp(rhs);
^
filesystem.cpp:230:41: note: in instantiation of function template specialization 'cpp11::named_arg::operator=<std::__1::shared_ptr<arrow::fs::FileSystem> >' requested here
return cpp11::writable::list({"fs"_nm = file_system, "path"_nm = out_path});
^
././arrow_cpp11.h:227:6: note: 'as_sexp' should be declared prior to the call site or in namespace 'arrow::fs'
SEXP as_sexp(const std::shared_ptr<T>& ptr) {
^
```
I'm not sure there is a cpp11 solution for this.
cc @jimhester
----------------------------------------------------------------
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 #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#discussion_r459671007
##########
File path: r/src/arrow_rcpp.h
##########
@@ -67,120 +71,150 @@ struct ns {
} // 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;
};
-namespace traits {
-
template <typename T>
-struct input_parameter<const std::shared_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<T>> type;
+class default_input {
+ public:
+ default_input(SEXP from) : from_(from) {}
+
+ inline operator T() const { return cpp11::as_cpp<T>(from_); }
+
+ private:
+ SEXP from_;
};
template <typename T>
-struct input_parameter<const std::unique_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::unique_ptr<T>> type;
+class const_reference_input {
+ public:
+ const_reference_input(SEXP from) : obj_(cpp11::as_cpp<T>(from)) {}
+
+ using const_reference = const T&;
+ inline operator const_reference() const { return obj_; }
Review comment:
```suggestion
operator const_reference() const { return obj_; }
```
##########
File path: r/src/memorypool.cpp
##########
@@ -19,6 +19,11 @@
#if defined(ARROW_R_WITH_ARROW)
#include <arrow/memory_pool.h>
+template <typename T>
+struct NoDelete {
+ inline void operator()(T* ptr) {}
+};
+
Review comment:
```suggestion
```
This can be replaced by a lambda
```c++
std::shared_ptr<arrow::MemoryPool> MemoryPool__default() {
return std::shared_ptr<arrow::MemoryPool>(arrow::default_memory_pool(),
[](arrow::MemoryPool* not_deleted){});
}
```
##########
File path: r/src/arrow_rcpp.h
##########
@@ -67,120 +71,150 @@ struct ns {
} // 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;
};
-namespace traits {
-
template <typename T>
-struct input_parameter<const std::shared_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<T>> type;
+class default_input {
+ public:
+ default_input(SEXP from) : from_(from) {}
+
+ inline operator T() const { return cpp11::as_cpp<T>(from_); }
Review comment:
```suggestion
operator T() const { return cpp11::as_cpp<T>(from_); }
```
member functions defined in the class body are implicitly inline
##########
File path: r/src/arrow_rcpp.h
##########
@@ -67,120 +71,150 @@ struct ns {
} // 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;
};
-namespace traits {
-
template <typename T>
-struct input_parameter<const std::shared_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<T>> type;
+class default_input {
+ public:
+ default_input(SEXP from) : from_(from) {}
+
+ inline operator T() const { return cpp11::as_cpp<T>(from_); }
+
+ private:
+ SEXP from_;
};
template <typename T>
-struct input_parameter<const std::unique_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::unique_ptr<T>> type;
+class const_reference_input {
+ public:
+ const_reference_input(SEXP from) : obj_(cpp11::as_cpp<T>(from)) {}
+
+ using const_reference = const T&;
+ inline operator const_reference() const { return obj_; }
+
+ private:
+ T obj_;
};
template <typename T>
-struct input_parameter<const std::vector<std::shared_ptr<T>>&> {
- typedef typename Rcpp::ConstReferenceVectorSmartPtrInputParameter<std::shared_ptr<T>>
- type;
+struct input {
+ using type = default_input<T>;
};
-struct wrap_type_shared_ptr_tag {};
-struct wrap_type_unique_ptr_tag {};
-
template <typename T>
-struct wrap_type_traits<std::shared_ptr<T>> {
- using wrap_category = wrap_type_shared_ptr_tag;
+struct input<const T&> {
+ using type = const_reference_input<typename std::decay<T>::type>;
};
template <typename T>
-struct wrap_type_traits<std::unique_ptr<T>> {
- using wrap_category = wrap_type_unique_ptr_tag;
+struct input<const std::shared_ptr<T>&> {
+ using type = ConstRefSmartPtrInput<T, std::shared_ptr>;
};
-} // namespace traits
-
-namespace internal {
+template <typename T>
+using default_unique_ptr = std::unique_ptr<T, std::default_delete<T>>;
template <typename T>
-inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_shared_ptr_tag);
+struct input<const std::unique_ptr<T>&> {
+ using type = ConstRefSmartPtrInput<T, default_unique_ptr>;
+};
template <typename T>
-inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_unique_ptr_tag);
+struct input<const std::vector<std::shared_ptr<T>>&> {
+ using type = ConstRefVectorSmartPtrInput<T, std::shared_ptr>;
+};
-} // namespace internal
-} // namespace Rcpp
+} // namespace r
+} // namespace arrow
-#include <Rcpp.h>
+namespace cpp11 {
-namespace Rcpp {
-namespace internal {
+template <typename T>
+using is_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_shared_ptr_tag) {
- return Rcpp::XPtr<std::shared_ptr<typename T::element_type>>(
- new std::shared_ptr<typename T::element_type>(x));
+is_shared_ptr<T> as_cpp(SEXP from) {
+ return arrow::r::ConstRefSmartPtrInput<typename T::element_type, std::shared_ptr>(from);
}
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()));
+inline SEXP as_sexp(const std::shared_ptr<T>& ptr) {
Review comment:
```suggestion
SEXP as_sexp(const std::shared_ptr<T>& ptr) {
```
templates functions are implicitly inline unless fully specialized
##########
File path: r/src/arrow_rcpp.h
##########
@@ -67,120 +71,150 @@ struct ns {
} // 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;
};
-namespace traits {
-
template <typename T>
-struct input_parameter<const std::shared_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<T>> type;
+class default_input {
+ public:
+ default_input(SEXP from) : from_(from) {}
Review comment:
```suggestion
explicit default_input(SEXP from) : from_(from) {}
```
##########
File path: r/src/arrow_rcpp.h
##########
@@ -67,120 +71,150 @@ struct ns {
} // 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;
};
-namespace traits {
-
template <typename T>
-struct input_parameter<const std::shared_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<T>> type;
+class default_input {
+ public:
+ default_input(SEXP from) : from_(from) {}
+
+ inline operator T() const { return cpp11::as_cpp<T>(from_); }
+
+ private:
+ SEXP from_;
};
template <typename T>
-struct input_parameter<const std::unique_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::unique_ptr<T>> type;
+class const_reference_input {
+ public:
+ const_reference_input(SEXP from) : obj_(cpp11::as_cpp<T>(from)) {}
+
+ using const_reference = const T&;
+ inline operator const_reference() const { return obj_; }
+
+ private:
+ T obj_;
};
template <typename T>
-struct input_parameter<const std::vector<std::shared_ptr<T>>&> {
- typedef typename Rcpp::ConstReferenceVectorSmartPtrInputParameter<std::shared_ptr<T>>
- type;
+struct input {
+ using type = default_input<T>;
};
-struct wrap_type_shared_ptr_tag {};
-struct wrap_type_unique_ptr_tag {};
-
template <typename T>
-struct wrap_type_traits<std::shared_ptr<T>> {
- using wrap_category = wrap_type_shared_ptr_tag;
+struct input<const T&> {
+ using type = const_reference_input<typename std::decay<T>::type>;
};
template <typename T>
-struct wrap_type_traits<std::unique_ptr<T>> {
- using wrap_category = wrap_type_unique_ptr_tag;
+struct input<const std::shared_ptr<T>&> {
+ using type = ConstRefSmartPtrInput<T, std::shared_ptr>;
};
-} // namespace traits
-
-namespace internal {
+template <typename T>
+using default_unique_ptr = std::unique_ptr<T, std::default_delete<T>>;
template <typename T>
-inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_shared_ptr_tag);
+struct input<const std::unique_ptr<T>&> {
+ using type = ConstRefSmartPtrInput<T, default_unique_ptr>;
+};
template <typename T>
-inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_unique_ptr_tag);
+struct input<const std::vector<std::shared_ptr<T>>&> {
+ using type = ConstRefVectorSmartPtrInput<T, std::shared_ptr>;
+};
-} // namespace internal
-} // namespace Rcpp
+} // namespace r
+} // namespace arrow
-#include <Rcpp.h>
+namespace cpp11 {
-namespace Rcpp {
-namespace internal {
+template <typename T>
+using is_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_shared_ptr_tag) {
- return Rcpp::XPtr<std::shared_ptr<typename T::element_type>>(
- new std::shared_ptr<typename T::element_type>(x));
+is_shared_ptr<T> as_cpp(SEXP from) {
Review comment:
```suggestion
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>
enable_if_shared_ptr<T> as_cpp(SEXP from) {
```
Names `is_*` are conventionally reserved for `bool_constant`s, the prefix `enable_if_` will make it clearer that this is an SFINAE helper
##########
File path: r/src/arrow_rcpp.h
##########
@@ -67,120 +71,150 @@ struct ns {
} // 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;
};
-namespace traits {
-
template <typename T>
-struct input_parameter<const std::shared_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<T>> type;
+class default_input {
+ public:
+ default_input(SEXP from) : from_(from) {}
Review comment:
Or if you need implicit conversion here,
```suggestion
default_input(SEXP from) : from_(from) {} // NOLINT runtime/explicit
```
##########
File path: r/src/arrow_rcpp.h
##########
@@ -67,120 +71,150 @@ struct ns {
} // 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;
};
-namespace traits {
-
template <typename T>
-struct input_parameter<const std::shared_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<T>> type;
+class default_input {
+ public:
+ default_input(SEXP from) : from_(from) {}
+
+ inline operator T() const { return cpp11::as_cpp<T>(from_); }
+
+ private:
+ SEXP from_;
};
template <typename T>
-struct input_parameter<const std::unique_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::unique_ptr<T>> type;
+class const_reference_input {
+ public:
+ const_reference_input(SEXP from) : obj_(cpp11::as_cpp<T>(from)) {}
Review comment:
```suggestion
explicit const_reference_input(SEXP from) : obj_(cpp11::as_cpp<T>(from)) {}
```
##########
File path: r/src/arrow_rcpp.h
##########
@@ -67,120 +71,150 @@ struct ns {
} // 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;
};
-namespace traits {
-
template <typename T>
-struct input_parameter<const std::shared_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<T>> type;
+class default_input {
+ public:
+ default_input(SEXP from) : from_(from) {}
+
+ inline operator T() const { return cpp11::as_cpp<T>(from_); }
+
+ private:
+ SEXP from_;
};
template <typename T>
-struct input_parameter<const std::unique_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::unique_ptr<T>> type;
+class const_reference_input {
+ public:
+ const_reference_input(SEXP from) : obj_(cpp11::as_cpp<T>(from)) {}
+
+ using const_reference = const T&;
+ inline operator const_reference() const { return obj_; }
+
+ private:
+ T obj_;
};
template <typename T>
-struct input_parameter<const std::vector<std::shared_ptr<T>>&> {
- typedef typename Rcpp::ConstReferenceVectorSmartPtrInputParameter<std::shared_ptr<T>>
- type;
+struct input {
+ using type = default_input<T>;
};
-struct wrap_type_shared_ptr_tag {};
-struct wrap_type_unique_ptr_tag {};
-
template <typename T>
-struct wrap_type_traits<std::shared_ptr<T>> {
- using wrap_category = wrap_type_shared_ptr_tag;
+struct input<const T&> {
+ using type = const_reference_input<typename std::decay<T>::type>;
};
template <typename T>
-struct wrap_type_traits<std::unique_ptr<T>> {
- using wrap_category = wrap_type_unique_ptr_tag;
+struct input<const std::shared_ptr<T>&> {
+ using type = ConstRefSmartPtrInput<T, std::shared_ptr>;
};
-} // namespace traits
-
-namespace internal {
+template <typename T>
+using default_unique_ptr = std::unique_ptr<T, std::default_delete<T>>;
template <typename T>
-inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_shared_ptr_tag);
+struct input<const std::unique_ptr<T>&> {
+ using type = ConstRefSmartPtrInput<T, default_unique_ptr>;
+};
template <typename T>
-inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_unique_ptr_tag);
+struct input<const std::vector<std::shared_ptr<T>>&> {
+ using type = ConstRefVectorSmartPtrInput<T, std::shared_ptr>;
+};
-} // namespace internal
-} // namespace Rcpp
+} // namespace r
+} // namespace arrow
-#include <Rcpp.h>
+namespace cpp11 {
-namespace Rcpp {
-namespace internal {
+template <typename T>
+using is_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_shared_ptr_tag) {
- return Rcpp::XPtr<std::shared_ptr<typename T::element_type>>(
- new std::shared_ptr<typename T::element_type>(x));
+is_shared_ptr<T> as_cpp(SEXP from) {
+ return arrow::r::ConstRefSmartPtrInput<typename T::element_type, std::shared_ptr>(from);
}
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()));
+inline SEXP as_sexp(const std::shared_ptr<T>& ptr) {
+ return Rcpp::XPtr<std::shared_ptr<T>>(new std::shared_ptr<T>(ptr));
}
-} // namespace internal
-
-} // namespace Rcpp
+template <typename T>
+inline SEXP 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 {
-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>;
+template <typename E, typename std::enable_if<std::is_enum<E>::value>::type* = nullptr>
+inline SEXP as_sexp(E e) {
Review comment:
```suggestion
SEXP as_sexp(E e) {
```
##########
File path: r/src/arrow_rcpp.h
##########
@@ -67,120 +71,150 @@ struct ns {
} // 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;
};
-namespace traits {
-
template <typename T>
-struct input_parameter<const std::shared_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<T>> type;
+class default_input {
+ public:
+ default_input(SEXP from) : from_(from) {}
+
+ inline operator T() const { return cpp11::as_cpp<T>(from_); }
+
+ private:
+ SEXP from_;
};
template <typename T>
-struct input_parameter<const std::unique_ptr<T>&> {
- typedef typename Rcpp::ConstReferenceSmartPtrInputParameter<std::unique_ptr<T>> type;
+class const_reference_input {
+ public:
+ const_reference_input(SEXP from) : obj_(cpp11::as_cpp<T>(from)) {}
+
+ using const_reference = const T&;
+ inline operator const_reference() const { return obj_; }
+
+ private:
+ T obj_;
};
template <typename T>
-struct input_parameter<const std::vector<std::shared_ptr<T>>&> {
- typedef typename Rcpp::ConstReferenceVectorSmartPtrInputParameter<std::shared_ptr<T>>
- type;
+struct input {
+ using type = default_input<T>;
};
-struct wrap_type_shared_ptr_tag {};
-struct wrap_type_unique_ptr_tag {};
-
template <typename T>
-struct wrap_type_traits<std::shared_ptr<T>> {
- using wrap_category = wrap_type_shared_ptr_tag;
+struct input<const T&> {
+ using type = const_reference_input<typename std::decay<T>::type>;
};
template <typename T>
-struct wrap_type_traits<std::unique_ptr<T>> {
- using wrap_category = wrap_type_unique_ptr_tag;
+struct input<const std::shared_ptr<T>&> {
+ using type = ConstRefSmartPtrInput<T, std::shared_ptr>;
};
-} // namespace traits
-
-namespace internal {
+template <typename T>
+using default_unique_ptr = std::unique_ptr<T, std::default_delete<T>>;
template <typename T>
-inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_shared_ptr_tag);
+struct input<const std::unique_ptr<T>&> {
+ using type = ConstRefSmartPtrInput<T, default_unique_ptr>;
+};
template <typename T>
-inline SEXP wrap_dispatch(const T& x, Rcpp::traits::wrap_type_unique_ptr_tag);
+struct input<const std::vector<std::shared_ptr<T>>&> {
+ using type = ConstRefVectorSmartPtrInput<T, std::shared_ptr>;
+};
-} // namespace internal
-} // namespace Rcpp
+} // namespace r
+} // namespace arrow
-#include <Rcpp.h>
+namespace cpp11 {
-namespace Rcpp {
-namespace internal {
+template <typename T>
+using is_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_shared_ptr_tag) {
- return Rcpp::XPtr<std::shared_ptr<typename T::element_type>>(
- new std::shared_ptr<typename T::element_type>(x));
+is_shared_ptr<T> as_cpp(SEXP from) {
+ return arrow::r::ConstRefSmartPtrInput<typename T::element_type, std::shared_ptr>(from);
}
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()));
+inline SEXP as_sexp(const std::shared_ptr<T>& ptr) {
+ return Rcpp::XPtr<std::shared_ptr<T>>(new std::shared_ptr<T>(ptr));
}
-} // namespace internal
-
-} // namespace Rcpp
+template <typename T>
+inline SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec) {
Review comment:
```suggestion
SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec) {
```
----------------------------------------------------------------
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] romainfrancois commented on pull request #7819: ARROW-9405: [R] Switch to cpp11
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #7819:
URL: https://github.com/apache/arrow/pull/7819#issuecomment-677645712
I think the last commit (https://github.com/apache/arrow/pull/7819/commits/23b179e7d69d6042821a41878b86e37b18cdeb44) restore how things were before switching to `cpp11` wrt how `uintptr_t` goes back and forth.
This uses a `double` as a host for a `uintptr_t` which IIUC is only faithful to 53 bits.
This should really use an external pointer but then I have no idea how python would go about extracting the actual pointer from the external pointer `SEXP`.
This seems to work, at least according to the tests, but I really don't know if the `uintptr_t` hosted in a `double` is fine with `x$`_export_to_c` expectations ?
cc @jjallaire @fsaintjacques @nealrichardson
```r
py_to_r.pyarrow.lib.Array <- function(x, ...) {
schema_ptr <- allocate_arrow_schema()
array_ptr <- allocate_arrow_array()
on.exit({
delete_arrow_schema(schema_ptr)
delete_arrow_array(array_ptr)
})
x$`_export_to_c`(array_ptr, schema_ptr)
Array$create(ImportArray(array_ptr, schema_ptr))
}
```
We get a `double` for both `schema_ptr` and `array_ptr` Is that ok ?
----------------------------------------------------------------
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