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.

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

Conversation

bkietz
Copy link
Collaborator

@bkietz bkietz commented Sep 17, 2020

Still a little half baked:

  • Move all conversion logic to an extensible trait structs
  • Generalize container conversion
  • Fix conversion from altrep vectors
  • Add doc indicating how and when to specialize the trait structs

@bkietz
Copy link
Collaborator Author

bkietz commented Sep 18, 2020

Odd: https://github.com/r-lib/cpp11/pull/104/checks?check_run_id=1130882732#step:12:198 what feature macro should I use to disable that test case? Also, why is the install cpp11 and cpp11test step marked successful (the failure is actually raised when trying to run the unbuilt tests)

@jimhester
Copy link
Member

You can use #if defined(__APPLE__) && defined(R_VERSION) && R_VERSION >= R_Version(3, 5, 0) to guard R_compact_intrange. ALTREP is only available on R 3.5.0+ and that symbol is hidden on linux and windows, but not on macOS, which is why we guard for apple.

utils::install.packages() the installation function shipped with base R only issues warnings when a package installation fails, not a true error.

d9eea11 should convert those warnings to failures and then fail the build in the cpp11test install step in the future.

@bkietz
Copy link
Collaborator Author

bkietz commented Sep 18, 2020

Thanks, I'll rebase

Comment on lines +246 to +248
expect_true(ALTREP(r));
auto x1 = cpp11::as_cpp<std::vector<int>>(r);
expect_true(ALTREP(r));
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;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants