Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor conversion to use extensible trait structs #104

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
+396 −250
Draft

Refactor conversion to use extensible trait structs #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
98a563a
Refactor conversion to use extensible trait structs
bkietz Sep 17, 2020
7d0867e
Add guard for ALTREP support
bkietz Sep 18, 2020
2feaefd
Don't use brace construction; changes behavior of Rcpp::List::List
bkietz Sep 18, 2020
bf45522
ensure as_sexp(r_string) doesn't depend on include order
bkietz Sep 18, 2020
580eb3b
add doc for conversion traits
bkietz Sep 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions cpp11test/src/test-as.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,24 @@

#include "Rcpp.h"

struct Vec3 {
double x, y, z;
};

namespace cpp11 {
template <>
struct as_cpp_impl<Vec3> {
static Vec3 convert(SEXP from) {
if (Rf_isReal(from)) {
if (Rf_xlength(from) == 3) {
return {REAL_ELT(from, 0), REAL_ELT(from, 1), REAL_ELT(from, 2)};
}
}
stop("Expected three double values");
}
};
} // namespace cpp11

context("as_cpp-C++") {
test_that("as_cpp<integer>(INTSEXP)") {
SEXP r = PROTECT(Rf_allocVector(INTSXP, 1));
Expand Down Expand Up @@ -57,6 +75,18 @@ context("as_cpp-C++") {
UNPROTECT(1);
}

test_that("as_cpp<Vec3>(REALSXP)") {
SEXP r = PROTECT(Rf_allocVector(REALSXP, 3));
REAL(r)[0] = 1;
REAL(r)[1] = 2;
REAL(r)[2] = 4;

Vec3 v = cpp11::as_cpp<Vec3>(r);
expect_true(v.x == 1);
expect_true(v.y == 2);
expect_true(v.z == 4);
}

test_that("as_cpp<integer>(NA)") {
SEXP r = PROTECT(Rf_allocVector(REALSXP, 1));
SEXP i = PROTECT(Rf_allocVector(INTSXP, 1));
Expand Down Expand Up @@ -209,6 +239,25 @@ context("as_cpp-C++") {
UNPROTECT(1);
}

#if defined(__APPLE__) && defined(R_VERSION) && R_VERSION >= R_Version(3, 5, 0)
test_that("as_cpp<std::vector>(ALTREP_INTSXP)") {
SEXP r = PROTECT(R_compact_intrange(1, 5));

expect_true(ALTREP(r));
auto x1 = cpp11::as_cpp<std::vector<int>>(r);
expect_true(ALTREP(r));
Comment on lines +246 to +248
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimhester I really expected something here to fail since as_cpp<IntegerContainer>() currently relies on INTEGER(), how is that working with ALTREP?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vector is still an altrep when you call INTEGER() on it, but it is then a materialized altrep vector.

The way to test that you are not materializing an alterep vector is to check that R_altrep_data2(x) == R_NilValue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. after doing this the second test fails, as you were expecting it to.

diff --git a/cpp11test/src/test-as.cpp b/cpp11test/src/test-as.cpp
index 6a3282c..0476ef2 100644
--- a/cpp11test/src/test-as.cpp
+++ b/cpp11test/src/test-as.cpp
@@ -243,9 +243,9 @@ context("as_cpp-C++") {
   test_that("as_cpp<std::vector>(ALTREP_INTSXP)") {
     SEXP r = PROTECT(R_compact_intrange(1, 5));
 
-    expect_true(ALTREP(r));
+    expect_true(R_altrep_data2(r) == R_NilValue);
     auto x1 = cpp11::as_cpp<std::vector<int>>(r);
-    expect_true(ALTREP(r));
+    expect_true(R_altrep_data2(r) == R_NilValue);
 
     expect_true(x1.size() == 5);
     int expected = 1;


expect_true(x1.size() == 5);
int expected = 1;
for (int actual : x1) {
expect_true(actual == expected);
++expected;
}

UNPROTECT(1);
}
#endif

test_that("as_cpp<std::vector<std::string>>()") {
SEXP r = PROTECT(Rf_allocVector(STRSXP, 3));
SET_STRING_ELT(r, 0, Rf_mkChar("foo"));
Expand Down
Loading