From 5590ec9f9ba53af790f3d75cf14c39a6190799e5 Mon Sep 17 00:00:00 2001 From: DavisVaughan Date: Thu, 3 Mar 2022 16:09:00 -0500 Subject: [PATCH 1/6] Correctly propagate missing values in `as_integers()` Also: - Switch to `R_xlen_t` over `size_t` since this is what `size()` returns and is what our constructors prefer - Initialize return value with `ret(len)` since that seems cleaner - Forward declare required bits from `doubles.hpp`. Use of `is_na()` is new, so it makes sense that we need to forward declare it. Even before this PR, we used the `[]` double operator, so you might be wondering why we need the forward declaration. Well, it only previously worked by luck because we had a hidden dependency on `cpp11/doubles.hpp` in `test-integers.cpp` by including `cpp11/doubles.hpp` before `cpp11/integers.hpp`. I swapped them around in the test file and it indeed failed without this forward declaration. --- cpp11test/src/test-integers.cpp | 7 ++++++- inst/include/cpp11/integers.hpp | 17 ++++++++++------- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/cpp11test/src/test-integers.cpp b/cpp11test/src/test-integers.cpp index 568e67eb..7f28a16d 100644 --- a/cpp11test/src/test-integers.cpp +++ b/cpp11test/src/test-integers.cpp @@ -1,6 +1,6 @@ #include "Rversion.h" -#include "cpp11/doubles.hpp" #include "cpp11/integers.hpp" +#include "cpp11/doubles.hpp" #include "cpp11/strings.hpp" #include @@ -35,6 +35,11 @@ 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/integers.hpp b/inst/include/cpp11/integers.hpp index 03420565..d4d9b49a 100644 --- a/inst/include/cpp11/integers.hpp +++ b/inst/include/cpp11/integers.hpp @@ -145,24 +145,27 @@ inline int na() { } // forward declaration - typedef r_vector doubles; +template <> bool is_na(const double& x); +template <> double r_vector::operator[](const R_xlen_t pos) const; inline integers as_integers(sexp x) { if (TYPEOF(x) == INTSXP) { return as_cpp(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) { + R_xlen_t len = xn.size(); + writable::integers ret(len); + for (R_xlen_t i = 0; i < len; ++i) { double el = xn[i]; - if (!is_convertable_without_loss_to_integer(el)) { + if (is_na(el)) { + ret[i] = na(); + } else if (is_convertable_without_loss_to_integer(el)) { + ret[i] = static_cast(el); + } else { throw std::runtime_error("All elements must be integer-like"); } - ret[i] = (static_cast(el)); } - return ret; } From 2fb620178c2b3f02083a3918f931e6c666222e1a Mon Sep 17 00:00:00 2001 From: DavisVaughan Date: Thu, 3 Mar 2022 16:14:05 -0500 Subject: [PATCH 2/6] Correctly propagate missing values in `as_doubles()` Also: - Switch to `R_xlen_t` over `size_t` since this is what `size()` returns and is what our constructors prefer - Initialize sized return value, rather than using `push_back()`, which should be faster - Forward declare required bits from `integers.hpp`. There is no `is_na()` specialization to forward declare, but there is a `na()` forward declaration that is required, because that is used by the default `is_na()` implementation - Move double specializations of `na()` and `is_na()` before the implementation of `as_doubles()`. Since `as_doubles()` now calls the `na()` specialization, it has to be implemented (or forward declared) ahead of time. --- cpp11test/src/test-doubles.cpp | 5 +++++ inst/include/cpp11/doubles.hpp | 39 ++++++++++++++++++++-------------- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/cpp11test/src/test-doubles.cpp b/cpp11test/src/test-doubles.cpp index bea555d0..3bdef104 100644 --- a/cpp11test/src/test-doubles.cpp +++ b/cpp11test/src/test-doubles.cpp @@ -379,6 +379,11 @@ 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/inst/include/cpp11/doubles.hpp b/inst/include/cpp11/doubles.hpp index 54924420..ef263246 100644 --- a/inst/include/cpp11/doubles.hpp +++ b/inst/include/cpp11/doubles.hpp @@ -134,19 +134,35 @@ 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 declaration 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); - size_t len = xn.size(); - writable::doubles ret; - for (size_t i = 0; i < len; ++i) { - ret.push_back(static_cast(xn[i])); + 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); + } } return ret; } @@ -154,13 +170,4 @@ 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 From d295081d5f1bb8b497c526f6fda78e6712084e08 Mon Sep 17 00:00:00 2001 From: DavisVaughan Date: Thu, 3 Mar 2022 16:15:08 -0500 Subject: [PATCH 3/6] NEWS bullet --- NEWS.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS.md b/NEWS.md index e4c4281e..859517a0 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). + * `cpp_source()` errors on non-existent file (#261). # cpp11 0.4.2 From d1b395a1aedb36f6cb39b4e29415fb5d5bf7a51a Mon Sep 17 00:00:00 2001 From: DavisVaughan Date: Thu, 3 Mar 2022 16:18:51 -0500 Subject: [PATCH 4/6] Guess at how to make the clang formatter happy --- inst/include/cpp11/doubles.hpp | 10 +++++++--- inst/include/cpp11/integers.hpp | 10 +++++++--- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/inst/include/cpp11/doubles.hpp b/inst/include/cpp11/doubles.hpp index ef263246..a9e7052e 100644 --- a/inst/include/cpp11/doubles.hpp +++ b/inst/include/cpp11/doubles.hpp @@ -144,10 +144,14 @@ inline bool is_na(const double& x) { return ISNA(x); } -// forward declaration +// forward declarations typedef r_vector integers; -template <> int na(); -template <> int r_vector::operator[](const R_xlen_t pos) const; + +template <> +int na(); + +template <> +int r_vector::operator[](const R_xlen_t pos) const; inline doubles as_doubles(sexp x) { if (TYPEOF(x) == REALSXP) { diff --git a/inst/include/cpp11/integers.hpp b/inst/include/cpp11/integers.hpp index d4d9b49a..3a382d81 100644 --- a/inst/include/cpp11/integers.hpp +++ b/inst/include/cpp11/integers.hpp @@ -144,10 +144,14 @@ inline int na() { return NA_INTEGER; } -// forward declaration +// forward declarations typedef r_vector doubles; -template <> bool is_na(const double& x); -template <> double r_vector::operator[](const R_xlen_t pos) const; + +template <> +bool is_na(const double& x); + +template <> +double r_vector::operator[](const R_xlen_t pos) const; inline integers as_integers(sexp x) { if (TYPEOF(x) == INTSXP) { From 457ca8220c0d0c8edcc11b23b937580f03a31cd6 Mon Sep 17 00:00:00 2001 From: DavisVaughan Date: Tue, 8 Mar 2022 12:29:45 -0500 Subject: [PATCH 5/6] Separate `cpp11/integers.hpp` into its own include "block" --- cpp11test/src/test-integers.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cpp11test/src/test-integers.cpp b/cpp11test/src/test-integers.cpp index 7f28a16d..a805fcda 100644 --- a/cpp11test/src/test-integers.cpp +++ b/cpp11test/src/test-integers.cpp @@ -1,5 +1,7 @@ #include "Rversion.h" + #include "cpp11/integers.hpp" + #include "cpp11/doubles.hpp" #include "cpp11/strings.hpp" From f32b8fa0143b842e4134789eb400ae03e2992794 Mon Sep 17 00:00:00 2001 From: DavisVaughan Date: Tue, 8 Mar 2022 12:31:08 -0500 Subject: [PATCH 6/6] Typo, `convertable` -> `convertible` --- 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 3a382d81..dea9d8e1 100644 --- a/inst/include/cpp11/integers.hpp +++ b/inst/include/cpp11/integers.hpp @@ -164,7 +164,7 @@ inline integers as_integers(sexp x) { double el = xn[i]; if (is_na(el)) { ret[i] = na(); - } else if (is_convertable_without_loss_to_integer(el)) { + } else if (is_convertible_without_loss_to_integer(el)) { ret[i] = static_cast(el); } else { throw std::runtime_error("All elements must be integer-like");