From 2df5613dae2c9962df1e3d69d3c1c54ea18dae9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Fran=C3=A7ois?= Date: Wed, 24 May 2023 11:50:55 +0200 Subject: [PATCH 01/10] Revert "Fix missing value propagation in `as_integers/doubles()` (#265)" This reverts commit 3c877986ce886b320ceafb853358d856f07834f4. --- NEWS.md | 3 --- cpp11test/src/test-doubles.cpp | 5 ---- cpp11test/src/test-integers.cpp | 9 +------ inst/include/cpp11/as.hpp | 4 +-- inst/include/cpp11/doubles.hpp | 43 ++++++++++++--------------------- inst/include/cpp11/integers.hpp | 23 ++++++------------ 6 files changed, 27 insertions(+), 60 deletions(-) diff --git a/NEWS.md b/NEWS.md index 85df4d74..ada3a2be 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,8 +1,5 @@ # cpp11 (development version) -* `as_doubles()` and `as_integers()` now propagate missing values correctly - (#265). - * Fixed a performance issue related to nested `unwind_protect()` calls (#298). * Minor performance improvements to the cpp11 protect code. (@kevinushey) diff --git a/cpp11test/src/test-doubles.cpp b/cpp11test/src/test-doubles.cpp index 2d847405..58023cd0 100644 --- a/cpp11test/src/test-doubles.cpp +++ b/cpp11test/src/test-doubles.cpp @@ -393,11 +393,6 @@ context("doubles-C++") { e.push_back("a"); e.push_back("b"); expect_error(cpp11::as_doubles(e)); - - cpp11::writable::integers na; - na.push_back(cpp11::na()); - cpp11::doubles na2(cpp11::as_doubles(na)); - expect_true(cpp11::is_na(na2[0])); } test_that("doubles operator[] and at") { diff --git a/cpp11test/src/test-integers.cpp b/cpp11test/src/test-integers.cpp index f2e6c0f7..83d2cccf 100644 --- a/cpp11test/src/test-integers.cpp +++ b/cpp11test/src/test-integers.cpp @@ -1,9 +1,7 @@ #include "Rversion.h" - +#include "cpp11/doubles.hpp" #include "cpp11/function.hpp" #include "cpp11/integers.hpp" - -#include "cpp11/doubles.hpp" #include "cpp11/strings.hpp" #include @@ -38,11 +36,6 @@ context("integers-C++") { expect_true(t[2] == 100000); expect_true(t[3] == 100000); expect_true(TYPEOF(t) == INTSXP); - - cpp11::writable::doubles na; - na.push_back(cpp11::na()); - cpp11::integers na2(cpp11::as_integers(na)); - expect_true(cpp11::is_na(na2[0])); } test_that("integers.push_back()") { diff --git a/inst/include/cpp11/as.hpp b/inst/include/cpp11/as.hpp index 682f12b5..6b4eb20f 100644 --- a/inst/include/cpp11/as.hpp +++ b/inst/include/cpp11/as.hpp @@ -73,7 +73,7 @@ using enable_if_c_string = enable_if_t::value, R>; // https://stackoverflow.com/a/1521682/2055486 // -inline bool is_convertible_without_loss_to_integer(double value) { +inline bool is_convertable_without_loss_to_integer(double value) { double int_part; return std::modf(value, &int_part) == 0.0; } @@ -100,7 +100,7 @@ enable_if_integral as_cpp(SEXP from) { return NA_INTEGER; } double value = REAL_ELT(from, 0); - if (is_convertible_without_loss_to_integer(value)) { + if (is_convertable_without_loss_to_integer(value)) { return value; } } diff --git a/inst/include/cpp11/doubles.hpp b/inst/include/cpp11/doubles.hpp index a20bdafa..f2d5db1f 100644 --- a/inst/include/cpp11/doubles.hpp +++ b/inst/include/cpp11/doubles.hpp @@ -135,39 +135,19 @@ typedef r_vector doubles; } // namespace writable -template <> -inline double na() { - return NA_REAL; -} - -template <> -inline bool is_na(const double& x) { - return ISNA(x); -} - -// forward declarations typedef r_vector integers; -template <> -int na(); - -template <> -int r_vector::operator[](const R_xlen_t pos) const; - inline doubles as_doubles(sexp x) { if (TYPEOF(x) == REALSXP) { return as_cpp(x); - } else if (TYPEOF(x) == INTSXP) { + } + + else if (TYPEOF(x) == INTSXP) { integers xn = as_cpp(x); - R_xlen_t len = xn.size(); - writable::doubles ret(len); - for (R_xlen_t i = 0; i < len; ++i) { - int el = xn[i]; - if (is_na(el)) { - ret[i] = na(); - } else { - ret[i] = static_cast(el); - } + size_t len = xn.size(); + writable::doubles ret; + for (size_t i = 0; i < len; ++i) { + ret.push_back(static_cast(xn[i])); } return ret; } @@ -175,4 +155,13 @@ inline doubles as_doubles(sexp x) { throw type_error(REALSXP, TYPEOF(x)); } +template <> +inline double na() { + return NA_REAL; +} + +template <> +inline bool is_na(const double& x) { + return ISNA(x); +} } // namespace cpp11 diff --git a/inst/include/cpp11/integers.hpp b/inst/include/cpp11/integers.hpp index 92b1fb12..657d93ff 100644 --- a/inst/include/cpp11/integers.hpp +++ b/inst/include/cpp11/integers.hpp @@ -145,32 +145,25 @@ inline int na() { return NA_INTEGER; } -// forward declarations -typedef r_vector doubles; - -template <> -bool is_na(const double& x); +// forward declaration -template <> -double r_vector::operator[](const R_xlen_t pos) const; +typedef r_vector doubles; inline integers as_integers(sexp x) { if (TYPEOF(x) == INTSXP) { return as_cpp(x); } else if (TYPEOF(x) == REALSXP) { doubles xn = as_cpp(x); - R_xlen_t len = xn.size(); - writable::integers ret(len); - for (R_xlen_t i = 0; i < len; ++i) { + size_t len = (xn.size()); + writable::integers ret = writable::integers(len); + for (size_t i = 0; i < len; ++i) { double el = xn[i]; - if (is_na(el)) { - ret[i] = na(); - } else if (is_convertible_without_loss_to_integer(el)) { - ret[i] = static_cast(el); - } else { + if (!is_convertable_without_loss_to_integer(el)) { throw std::runtime_error("All elements must be integer-like"); } + ret[i] = (static_cast(el)); } + return ret; } From 04f3d6244439cbb8b5caa2474e0e1428c85767fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Fran=C3=A7ois?= Date: Wed, 24 May 2023 14:50:15 +0200 Subject: [PATCH 02/10] is_convertible_without_loss_to_integer --- inst/include/cpp11/as.hpp | 4 ++-- inst/include/cpp11/integers.hpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/inst/include/cpp11/as.hpp b/inst/include/cpp11/as.hpp index 6b4eb20f..682f12b5 100644 --- a/inst/include/cpp11/as.hpp +++ b/inst/include/cpp11/as.hpp @@ -73,7 +73,7 @@ using enable_if_c_string = enable_if_t::value, R>; // https://stackoverflow.com/a/1521682/2055486 // -inline bool is_convertable_without_loss_to_integer(double value) { +inline bool is_convertible_without_loss_to_integer(double value) { double int_part; return std::modf(value, &int_part) == 0.0; } @@ -100,7 +100,7 @@ enable_if_integral as_cpp(SEXP from) { return NA_INTEGER; } double value = REAL_ELT(from, 0); - if (is_convertable_without_loss_to_integer(value)) { + if (is_convertible_without_loss_to_integer(value)) { return value; } } diff --git a/inst/include/cpp11/integers.hpp b/inst/include/cpp11/integers.hpp index 657d93ff..c3a89c8e 100644 --- a/inst/include/cpp11/integers.hpp +++ b/inst/include/cpp11/integers.hpp @@ -158,7 +158,7 @@ inline integers as_integers(sexp x) { writable::integers ret = writable::integers(len); for (size_t i = 0; i < len; ++i) { double el = xn[i]; - if (!is_convertable_without_loss_to_integer(el)) { + if (!is_convertible_without_loss_to_integer(el)) { throw std::runtime_error("All elements must be integer-like"); } ret[i] = (static_cast(el)); From 3a72a7a431ae8bdb99e1bb3c63baefdd4cb40fb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Fran=C3=A7ois?= Date: Wed, 24 May 2023 14:58:29 +0200 Subject: [PATCH 03/10] is_na using sfinae to differentiate double and !double --- cpp11test/R/cpp11.R | 104 +++++++++--------- cpp11test/src/cpp11.cpp | 194 ++++++++++++++++----------------- inst/include/cpp11/R.hpp | 12 +- inst/include/cpp11/doubles.hpp | 4 - 4 files changed, 160 insertions(+), 154 deletions(-) diff --git a/cpp11test/R/cpp11.R b/cpp11test/R/cpp11.R index 3d6b0f5d..4b9a1d9a 100644 --- a/cpp11test/R/cpp11.R +++ b/cpp11test/R/cpp11.R @@ -8,30 +8,6 @@ data_frame_ <- function() { .Call(`_cpp11test_data_frame_`) } -my_stop <- function(mystring, myarg) { - invisible(.Call(`_cpp11test_my_stop`, mystring, myarg)) -} - -my_stop_n1 <- function(mystring) { - invisible(.Call(`_cpp11test_my_stop_n1`, mystring)) -} - -my_warning <- function(mystring, myarg) { - invisible(.Call(`_cpp11test_my_warning`, mystring, myarg)) -} - -my_warning_n1 <- function(mystring) { - invisible(.Call(`_cpp11test_my_warning_n1`, mystring)) -} - -my_message <- function(mystring, myarg) { - invisible(.Call(`_cpp11test_my_message`, mystring, myarg)) -} - -my_message_n1 <- function(mystring) { - invisible(.Call(`_cpp11test_my_message_n1`, mystring)) -} - my_stop_n1fmt <- function(mystring) { invisible(.Call(`_cpp11test_my_stop_n1fmt`, mystring)) } @@ -56,6 +32,30 @@ my_message_n2fmt <- function(mystring, myarg) { invisible(.Call(`_cpp11test_my_message_n2fmt`, mystring, myarg)) } +my_stop <- function(mystring, myarg) { + invisible(.Call(`_cpp11test_my_stop`, mystring, myarg)) +} + +my_stop_n1 <- function(mystring) { + invisible(.Call(`_cpp11test_my_stop_n1`, mystring)) +} + +my_warning <- function(mystring, myarg) { + invisible(.Call(`_cpp11test_my_warning`, mystring, myarg)) +} + +my_warning_n1 <- function(mystring) { + invisible(.Call(`_cpp11test_my_warning_n1`, mystring)) +} + +my_message <- function(mystring, myarg) { + invisible(.Call(`_cpp11test_my_message`, mystring, myarg)) +} + +my_message_n1 <- function(mystring) { + invisible(.Call(`_cpp11test_my_message_n1`, mystring)) +} + remove_altrep <- function(x) { .Call(`_cpp11test_remove_altrep`, x) } @@ -160,6 +160,34 @@ cpp11_safe_ <- function(x_sxp) { .Call(`_cpp11test_cpp11_safe_`, x_sxp) } +sum_dbl_for_ <- function(x) { + .Call(`_cpp11test_sum_dbl_for_`, x) +} + +sum_dbl_for2_ <- function(x_sxp) { + .Call(`_cpp11test_sum_dbl_for2_`, x_sxp) +} + +sum_dbl_for3_ <- function(x_sxp) { + .Call(`_cpp11test_sum_dbl_for3_`, x_sxp) +} + +sum_dbl_foreach_ <- function(x) { + .Call(`_cpp11test_sum_dbl_foreach_`, x) +} + +sum_dbl_foreach2_ <- function(x_sxp) { + .Call(`_cpp11test_sum_dbl_foreach2_`, x_sxp) +} + +sum_dbl_accumulate_ <- function(x) { + .Call(`_cpp11test_sum_dbl_accumulate_`, x) +} + +sum_dbl_accumulate2_ <- function(x_sxp) { + .Call(`_cpp11test_sum_dbl_accumulate2_`, x_sxp) +} + sum_int_for_ <- function(x) { .Call(`_cpp11test_sum_int_for_`, x) } @@ -195,31 +223,3 @@ rcpp_sum_dbl_accumulate_ <- function(x_sxp) { rcpp_grow_ <- function(n_sxp) { .Call(`_cpp11test_rcpp_grow_`, n_sxp) } - -sum_dbl_for_ <- function(x) { - .Call(`_cpp11test_sum_dbl_for_`, x) -} - -sum_dbl_for2_ <- function(x_sxp) { - .Call(`_cpp11test_sum_dbl_for2_`, x_sxp) -} - -sum_dbl_for3_ <- function(x_sxp) { - .Call(`_cpp11test_sum_dbl_for3_`, x_sxp) -} - -sum_dbl_foreach_ <- function(x) { - .Call(`_cpp11test_sum_dbl_foreach_`, x) -} - -sum_dbl_foreach2_ <- function(x_sxp) { - .Call(`_cpp11test_sum_dbl_foreach2_`, x_sxp) -} - -sum_dbl_accumulate_ <- function(x) { - .Call(`_cpp11test_sum_dbl_accumulate_`, x) -} - -sum_dbl_accumulate2_ <- function(x_sxp) { - .Call(`_cpp11test_sum_dbl_accumulate2_`, x_sxp) -} diff --git a/cpp11test/src/cpp11.cpp b/cpp11test/src/cpp11.cpp index d38863ee..44472d85 100644 --- a/cpp11test/src/cpp11.cpp +++ b/cpp11test/src/cpp11.cpp @@ -21,54 +21,6 @@ extern "C" SEXP _cpp11test_data_frame_() { return cpp11::as_sexp(data_frame_()); END_CPP11 } -// errors_fmt.cpp -void my_stop(std::string mystring, int myarg); -extern "C" SEXP _cpp11test_my_stop(SEXP mystring, SEXP myarg) { - BEGIN_CPP11 - my_stop(cpp11::as_cpp>(mystring), cpp11::as_cpp>(myarg)); - return R_NilValue; - END_CPP11 -} -// errors_fmt.cpp -void my_stop_n1(std::string mystring); -extern "C" SEXP _cpp11test_my_stop_n1(SEXP mystring) { - BEGIN_CPP11 - my_stop_n1(cpp11::as_cpp>(mystring)); - return R_NilValue; - END_CPP11 -} -// errors_fmt.cpp -void my_warning(std::string mystring, std::string myarg); -extern "C" SEXP _cpp11test_my_warning(SEXP mystring, SEXP myarg) { - BEGIN_CPP11 - my_warning(cpp11::as_cpp>(mystring), cpp11::as_cpp>(myarg)); - return R_NilValue; - END_CPP11 -} -// errors_fmt.cpp -void my_warning_n1(std::string mystring); -extern "C" SEXP _cpp11test_my_warning_n1(SEXP mystring) { - BEGIN_CPP11 - my_warning_n1(cpp11::as_cpp>(mystring)); - return R_NilValue; - END_CPP11 -} -// errors_fmt.cpp -void my_message(std::string mystring, std::string myarg); -extern "C" SEXP _cpp11test_my_message(SEXP mystring, SEXP myarg) { - BEGIN_CPP11 - my_message(cpp11::as_cpp>(mystring), cpp11::as_cpp>(myarg)); - return R_NilValue; - END_CPP11 -} -// errors_fmt.cpp -void my_message_n1(std::string mystring); -extern "C" SEXP _cpp11test_my_message_n1(SEXP mystring) { - BEGIN_CPP11 - my_message_n1(cpp11::as_cpp>(mystring)); - return R_NilValue; - END_CPP11 -} // errors.cpp void my_stop_n1fmt(std::string mystring); extern "C" SEXP _cpp11test_my_stop_n1fmt(SEXP mystring) { @@ -117,6 +69,54 @@ extern "C" SEXP _cpp11test_my_message_n2fmt(SEXP mystring, SEXP myarg) { return R_NilValue; END_CPP11 } +// errors_fmt.cpp +void my_stop(std::string mystring, int myarg); +extern "C" SEXP _cpp11test_my_stop(SEXP mystring, SEXP myarg) { + BEGIN_CPP11 + my_stop(cpp11::as_cpp>(mystring), cpp11::as_cpp>(myarg)); + return R_NilValue; + END_CPP11 +} +// errors_fmt.cpp +void my_stop_n1(std::string mystring); +extern "C" SEXP _cpp11test_my_stop_n1(SEXP mystring) { + BEGIN_CPP11 + my_stop_n1(cpp11::as_cpp>(mystring)); + return R_NilValue; + END_CPP11 +} +// errors_fmt.cpp +void my_warning(std::string mystring, std::string myarg); +extern "C" SEXP _cpp11test_my_warning(SEXP mystring, SEXP myarg) { + BEGIN_CPP11 + my_warning(cpp11::as_cpp>(mystring), cpp11::as_cpp>(myarg)); + return R_NilValue; + END_CPP11 +} +// errors_fmt.cpp +void my_warning_n1(std::string mystring); +extern "C" SEXP _cpp11test_my_warning_n1(SEXP mystring) { + BEGIN_CPP11 + my_warning_n1(cpp11::as_cpp>(mystring)); + return R_NilValue; + END_CPP11 +} +// errors_fmt.cpp +void my_message(std::string mystring, std::string myarg); +extern "C" SEXP _cpp11test_my_message(SEXP mystring, SEXP myarg) { + BEGIN_CPP11 + my_message(cpp11::as_cpp>(mystring), cpp11::as_cpp>(myarg)); + return R_NilValue; + END_CPP11 +} +// errors_fmt.cpp +void my_message_n1(std::string mystring); +extern "C" SEXP _cpp11test_my_message_n1(SEXP mystring) { + BEGIN_CPP11 + my_message_n1(cpp11::as_cpp>(mystring)); + return R_NilValue; + END_CPP11 +} // find-intervals.cpp SEXP remove_altrep(SEXP x); extern "C" SEXP _cpp11test_remove_altrep(SEXP x) { @@ -310,6 +310,55 @@ extern "C" SEXP _cpp11test_cpp11_safe_(SEXP x_sxp) { return cpp11::as_sexp(cpp11_safe_(cpp11::as_cpp>(x_sxp))); END_CPP11 } +// sum.cpp +double sum_dbl_for_(cpp11::doubles x); +extern "C" SEXP _cpp11test_sum_dbl_for_(SEXP x) { + BEGIN_CPP11 + return cpp11::as_sexp(sum_dbl_for_(cpp11::as_cpp>(x))); + END_CPP11 +} +// sum.cpp +double sum_dbl_for2_(SEXP x_sxp); +extern "C" SEXP _cpp11test_sum_dbl_for2_(SEXP x_sxp) { + BEGIN_CPP11 + return cpp11::as_sexp(sum_dbl_for2_(cpp11::as_cpp>(x_sxp))); + END_CPP11 +} +// sum.cpp +double sum_dbl_for3_(SEXP x_sxp); +extern "C" SEXP _cpp11test_sum_dbl_for3_(SEXP x_sxp) { + BEGIN_CPP11 + return cpp11::as_sexp(sum_dbl_for3_(cpp11::as_cpp>(x_sxp))); + END_CPP11 +} +// sum.cpp +double sum_dbl_foreach_(cpp11::doubles x); +extern "C" SEXP _cpp11test_sum_dbl_foreach_(SEXP x) { + BEGIN_CPP11 + return cpp11::as_sexp(sum_dbl_foreach_(cpp11::as_cpp>(x))); + END_CPP11 +} +// sum.cpp +double sum_dbl_foreach2_(SEXP x_sxp); +extern "C" SEXP _cpp11test_sum_dbl_foreach2_(SEXP x_sxp) { + BEGIN_CPP11 + return cpp11::as_sexp(sum_dbl_foreach2_(cpp11::as_cpp>(x_sxp))); + END_CPP11 +} +// sum.cpp +double sum_dbl_accumulate_(cpp11::doubles x); +extern "C" SEXP _cpp11test_sum_dbl_accumulate_(SEXP x) { + BEGIN_CPP11 + return cpp11::as_sexp(sum_dbl_accumulate_(cpp11::as_cpp>(x))); + END_CPP11 +} +// sum.cpp +double sum_dbl_accumulate2_(SEXP x_sxp); +extern "C" SEXP _cpp11test_sum_dbl_accumulate2_(SEXP x_sxp) { + BEGIN_CPP11 + return cpp11::as_sexp(sum_dbl_accumulate2_(cpp11::as_cpp>(x_sxp))); + END_CPP11 +} // sum_int.cpp double sum_int_for_(cpp11::integers x); extern "C" SEXP _cpp11test_sum_int_for_(SEXP x) { @@ -373,55 +422,6 @@ extern "C" SEXP _cpp11test_rcpp_grow_(SEXP n_sxp) { return cpp11::as_sexp(rcpp_grow_(cpp11::as_cpp>(n_sxp))); END_CPP11 } -// sum.cpp -double sum_dbl_for_(cpp11::doubles x); -extern "C" SEXP _cpp11test_sum_dbl_for_(SEXP x) { - BEGIN_CPP11 - return cpp11::as_sexp(sum_dbl_for_(cpp11::as_cpp>(x))); - END_CPP11 -} -// sum.cpp -double sum_dbl_for2_(SEXP x_sxp); -extern "C" SEXP _cpp11test_sum_dbl_for2_(SEXP x_sxp) { - BEGIN_CPP11 - return cpp11::as_sexp(sum_dbl_for2_(cpp11::as_cpp>(x_sxp))); - END_CPP11 -} -// sum.cpp -double sum_dbl_for3_(SEXP x_sxp); -extern "C" SEXP _cpp11test_sum_dbl_for3_(SEXP x_sxp) { - BEGIN_CPP11 - return cpp11::as_sexp(sum_dbl_for3_(cpp11::as_cpp>(x_sxp))); - END_CPP11 -} -// sum.cpp -double sum_dbl_foreach_(cpp11::doubles x); -extern "C" SEXP _cpp11test_sum_dbl_foreach_(SEXP x) { - BEGIN_CPP11 - return cpp11::as_sexp(sum_dbl_foreach_(cpp11::as_cpp>(x))); - END_CPP11 -} -// sum.cpp -double sum_dbl_foreach2_(SEXP x_sxp); -extern "C" SEXP _cpp11test_sum_dbl_foreach2_(SEXP x_sxp) { - BEGIN_CPP11 - return cpp11::as_sexp(sum_dbl_foreach2_(cpp11::as_cpp>(x_sxp))); - END_CPP11 -} -// sum.cpp -double sum_dbl_accumulate_(cpp11::doubles x); -extern "C" SEXP _cpp11test_sum_dbl_accumulate_(SEXP x) { - BEGIN_CPP11 - return cpp11::as_sexp(sum_dbl_accumulate_(cpp11::as_cpp>(x))); - END_CPP11 -} -// sum.cpp -double sum_dbl_accumulate2_(SEXP x_sxp); -extern "C" SEXP _cpp11test_sum_dbl_accumulate2_(SEXP x_sxp) { - BEGIN_CPP11 - return cpp11::as_sexp(sum_dbl_accumulate2_(cpp11::as_cpp>(x_sxp))); - END_CPP11 -} extern "C" { /* .Call calls */ diff --git a/inst/include/cpp11/R.hpp b/inst/include/cpp11/R.hpp index 59054591..4c2c4fa3 100644 --- a/inst/include/cpp11/R.hpp +++ b/inst/include/cpp11/R.hpp @@ -28,6 +28,7 @@ #endif // clang-format on +#include #include "cpp11/altrep.hpp" namespace cpp11 { @@ -48,8 +49,17 @@ template inline T na(); template -inline bool is_na(const T& value) { +inline typename std::enable_if::type, double>::value, + bool>::type +is_na(const T& value) { return value == na(); } +template +inline typename std::enable_if::type, double>::value, + bool>::type +is_na(const T& value) { + return ISNA(value); +} + } // namespace cpp11 diff --git a/inst/include/cpp11/doubles.hpp b/inst/include/cpp11/doubles.hpp index f2d5db1f..a55c8548 100644 --- a/inst/include/cpp11/doubles.hpp +++ b/inst/include/cpp11/doubles.hpp @@ -160,8 +160,4 @@ inline double na() { return NA_REAL; } -template <> -inline bool is_na(const double& x) { - return ISNA(x); -} } // namespace cpp11 From 47e3fac716a119edd280ad203838752706882733 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Fran=C3=A7ois?= Date: Thu, 25 May 2023 09:16:58 +0200 Subject: [PATCH 04/10] install local cpp11 before cpp11test --- .github/workflows/R-CMD-check.yaml | 1 + cpp11test/src/test-doubles.cpp | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml index 8505e245..7932058a 100644 --- a/.github/workflows/R-CMD-check.yaml +++ b/.github/workflows/R-CMD-check.yaml @@ -85,6 +85,7 @@ jobs: run: | options(warn = 2) pak::local_install_dev_deps("cpp11test", dependencies = TRUE) + install.packages(".", repos = NULL, type = "source") install.packages("cpp11test", repos = NULL, INSTALL_opts = "--install-tests", type = "source") shell: Rscript {0} diff --git a/cpp11test/src/test-doubles.cpp b/cpp11test/src/test-doubles.cpp index 58023cd0..ba4f5ab8 100644 --- a/cpp11test/src/test-doubles.cpp +++ b/cpp11test/src/test-doubles.cpp @@ -393,6 +393,10 @@ context("doubles-C++") { e.push_back("a"); e.push_back("b"); expect_error(cpp11::as_doubles(e)); + + cpp11::writable::integers na {NA_INTEGER}; + cpp11::doubles na2(cpp11::as_doubles(na)); + expect_true(cpp11::is_na(na2[0])); } test_that("doubles operator[] and at") { From 53b3de36ee931aca5f751610337d2d4661e0bb5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Fran=C3=A7ois?= Date: Thu, 25 May 2023 09:21:17 +0200 Subject: [PATCH 05/10] make format --- Makefile | 4 ---- cpp11test/src/test-doubles.cpp | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/Makefile b/Makefile index 91261c66..d99155d2 100644 --- a/Makefile +++ b/Makefile @@ -13,8 +13,4 @@ clean: clang_format=`which clang-format` format: $(shell find . -name '*.hpp') $(shell find . -name '*.cpp') -ifeq ($(findstring version 10,$(shell ${clang_format} --version 2>/dev/null)),) - @echo "clang-format 10 is required" -else @${clang_format} -i $? -endif diff --git a/cpp11test/src/test-doubles.cpp b/cpp11test/src/test-doubles.cpp index ba4f5ab8..0a444a95 100644 --- a/cpp11test/src/test-doubles.cpp +++ b/cpp11test/src/test-doubles.cpp @@ -394,7 +394,7 @@ context("doubles-C++") { e.push_back("b"); expect_error(cpp11::as_doubles(e)); - cpp11::writable::integers na {NA_INTEGER}; + cpp11::writable::integers na{NA_INTEGER}; cpp11::doubles na2(cpp11::as_doubles(na)); expect_true(cpp11::is_na(na2[0])); } From 30a4909d3661527c246e2de70a6d74391df3b513 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Fran=C3=A7ois?= Date: Thu, 25 May 2023 09:36:33 +0200 Subject: [PATCH 06/10] propage na --- inst/include/cpp11/doubles.hpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/inst/include/cpp11/doubles.hpp b/inst/include/cpp11/doubles.hpp index a55c8548..754e77cf 100644 --- a/inst/include/cpp11/doubles.hpp +++ b/inst/include/cpp11/doubles.hpp @@ -1,6 +1,6 @@ #pragma once -#include // for min +#include // for min, tranform #include // for array #include // for initializer_list @@ -139,16 +139,16 @@ typedef r_vector integers; inline doubles as_doubles(sexp x) { if (TYPEOF(x) == REALSXP) { - return as_cpp(x); + return doubles(x); } else if (TYPEOF(x) == INTSXP) { - integers xn = as_cpp(x); + integers xn(x); size_t len = xn.size(); - writable::doubles ret; - for (size_t i = 0; i < len; ++i) { - ret.push_back(static_cast(xn[i])); - } + writable::doubles ret(len); + std::transform(xn.begin(), xn.end(), ret.begin(), [](int value) { + return value == NA_INTEGER ? NA_REAL : static_cast(value); + }); return ret; } From 09c8ce526afd3e3bc3eafd9af00b37bbaa7a42e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Fran=C3=A7ois?= Date: Thu, 25 May 2023 10:07:50 +0200 Subject: [PATCH 07/10] integers test too --- cpp11test/src/test-doubles.cpp | 3 ++- cpp11test/src/test-integers.cpp | 6 ++++++ inst/include/cpp11/integers.hpp | 17 +++++++---------- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/cpp11test/src/test-doubles.cpp b/cpp11test/src/test-doubles.cpp index 0a444a95..a7423e1e 100644 --- a/cpp11test/src/test-doubles.cpp +++ b/cpp11test/src/test-doubles.cpp @@ -394,9 +394,10 @@ context("doubles-C++") { e.push_back("b"); expect_error(cpp11::as_doubles(e)); - cpp11::writable::integers na{NA_INTEGER}; + cpp11::writable::integers na{NA_INTEGER, 42}; cpp11::doubles na2(cpp11::as_doubles(na)); expect_true(cpp11::is_na(na2[0])); + expect_true(!cpp11::is_na(na2[1])); } test_that("doubles operator[] and at") { diff --git a/cpp11test/src/test-integers.cpp b/cpp11test/src/test-integers.cpp index 83d2cccf..cdb48c1e 100644 --- a/cpp11test/src/test-integers.cpp +++ b/cpp11test/src/test-integers.cpp @@ -36,6 +36,12 @@ context("integers-C++") { expect_true(t[2] == 100000); expect_true(t[3] == 100000); expect_true(TYPEOF(t) == INTSXP); + + cpp11::writable::doubles na{NA_REAL, (double)NA_INTEGER, 42.}; + cpp11::integers na2(cpp11::as_integers(na)); + expect_true(cpp11::is_na(na2[0])); + expect_true(!cpp11::is_na(na2[1])); + expect_true(!cpp11::is_na(na2[2])); } test_that("integers.push_back()") { diff --git a/inst/include/cpp11/integers.hpp b/inst/include/cpp11/integers.hpp index c3a89c8e..1d0ff74e 100644 --- a/inst/include/cpp11/integers.hpp +++ b/inst/include/cpp11/integers.hpp @@ -151,19 +151,16 @@ typedef r_vector doubles; inline integers as_integers(sexp x) { if (TYPEOF(x) == INTSXP) { - return as_cpp(x); + return integers(x); } else if (TYPEOF(x) == REALSXP) { - doubles xn = as_cpp(x); - size_t len = (xn.size()); - writable::integers ret = writable::integers(len); - for (size_t i = 0; i < len; ++i) { - double el = xn[i]; - if (!is_convertible_without_loss_to_integer(el)) { + doubles xn(x); + writable::integers ret = writable::integers(xn.size()); + std::transform(xn.begin(), xn.end(), ret.begin(), [](double value) { + if (!is_convertible_without_loss_to_integer(value)) { throw std::runtime_error("All elements must be integer-like"); } - ret[i] = (static_cast(el)); - } - + return ISNA(value) ? NA_INTEGER : static_cast(value); + }); return ret; } From b0d311e7ed222f0f65db16181d406c29ed67b0cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Fran=C3=A7ois?= Date: Thu, 25 May 2023 10:08:54 +0200 Subject: [PATCH 08/10] new bullet --- NEWS.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS.md b/NEWS.md index ada3a2be..3565b546 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # cpp11 (development version) +* `as_doubles()` and `as_integers()` now propagate missing values correctly + (#265, #319). + * Fixed a performance issue related to nested `unwind_protect()` calls (#298). * Minor performance improvements to the cpp11 protect code. (@kevinushey) From d56e3e6041b6bf3736432ce3c827377bc910e9b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Fran=C3=A7ois?= Date: Thu, 25 May 2023 10:23:29 +0200 Subject: [PATCH 09/10] as_doubles(SEXP) --- cpp11test/src/test-integers.cpp | 3 +-- inst/include/cpp11/doubles.hpp | 2 +- inst/include/cpp11/integers.hpp | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/cpp11test/src/test-integers.cpp b/cpp11test/src/test-integers.cpp index cdb48c1e..6f876c57 100644 --- a/cpp11test/src/test-integers.cpp +++ b/cpp11test/src/test-integers.cpp @@ -37,11 +37,10 @@ context("integers-C++") { expect_true(t[3] == 100000); expect_true(TYPEOF(t) == INTSXP); - cpp11::writable::doubles na{NA_REAL, (double)NA_INTEGER, 42.}; + cpp11::writable::doubles na{NA_REAL, 42.}; cpp11::integers na2(cpp11::as_integers(na)); expect_true(cpp11::is_na(na2[0])); expect_true(!cpp11::is_na(na2[1])); - expect_true(!cpp11::is_na(na2[2])); } test_that("integers.push_back()") { diff --git a/inst/include/cpp11/doubles.hpp b/inst/include/cpp11/doubles.hpp index 754e77cf..9782346d 100644 --- a/inst/include/cpp11/doubles.hpp +++ b/inst/include/cpp11/doubles.hpp @@ -137,7 +137,7 @@ typedef r_vector doubles; typedef r_vector integers; -inline doubles as_doubles(sexp x) { +inline doubles as_doubles(SEXP x) { if (TYPEOF(x) == REALSXP) { return doubles(x); } diff --git a/inst/include/cpp11/integers.hpp b/inst/include/cpp11/integers.hpp index 1d0ff74e..63e951da 100644 --- a/inst/include/cpp11/integers.hpp +++ b/inst/include/cpp11/integers.hpp @@ -149,7 +149,7 @@ inline int na() { typedef r_vector doubles; -inline integers as_integers(sexp x) { +inline integers as_integers(SEXP x) { if (TYPEOF(x) == INTSXP) { return integers(x); } else if (TYPEOF(x) == REALSXP) { From 569c5f6b64757ff6c489d7f4ee45087d25e6bf2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Fran=C3=A7ois?= Date: Thu, 25 May 2023 10:52:31 +0200 Subject: [PATCH 10/10] test for NA_REAL first --- Makefile | 1 + inst/include/cpp11/integers.hpp | 7 +++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index d99155d2..4b85c9f1 100644 --- a/Makefile +++ b/Makefile @@ -10,6 +10,7 @@ test: all clean: @Rscript -e 'devtools::clean_dll()' + @Rscript -e 'devtools::clean_dll("cpp11test")' clang_format=`which clang-format` format: $(shell find . -name '*.hpp') $(shell find . -name '*.cpp') diff --git a/inst/include/cpp11/integers.hpp b/inst/include/cpp11/integers.hpp index 63e951da..1159e2f3 100644 --- a/inst/include/cpp11/integers.hpp +++ b/inst/include/cpp11/integers.hpp @@ -154,12 +154,15 @@ inline integers as_integers(SEXP x) { return integers(x); } else if (TYPEOF(x) == REALSXP) { doubles xn(x); - writable::integers ret = writable::integers(xn.size()); + writable::integers ret(xn.size()); std::transform(xn.begin(), xn.end(), ret.begin(), [](double value) { + if (ISNA(value)) { + return NA_INTEGER; + } if (!is_convertible_without_loss_to_integer(value)) { throw std::runtime_error("All elements must be integer-like"); } - return ISNA(value) ? NA_INTEGER : static_cast(value); + return static_cast(value); }); return ret; }