Add the Dataptr* methods to ALTREP representation #286
Conversation
…nside `mclapply()`)
There was a problem hiding this comment.
Thanks!
It's tricky to test this, but I believe that the_list[[0]] <- the_list[[1]] will trigger SetElt. For strings sort() and unique() will trigger Dataptr() but I think those will just error here. Perhaps a stub Rcpp function whose only job is to call DATAPTR_RO(input) would be a way to make sure this works from our test suite.
src/s2-altrep.cpp
Outdated
| } | ||
|
|
||
| static void* s2_altrep_Dataptr(SEXP obj, Rboolean writable) { | ||
| if (writable) return NULL; |
There was a problem hiding this comment.
I am not sure that you can return NULL here. I don't think it's harmful to skip this check, though, since we allocate all of the lists that we wrap in this ALTREP class.
There was a problem hiding this comment.
It sounds like a defect in R spec to me. Technically, DATAPTR is non-api (so I can't use it here), and returning writeable pointers for ALTREP lists is not supported at the API level. Since we seem to be entering the realm of undefined behavior, I think that returning NULL is fine. I also dislike casing away const pointer, but I don't really see a way around it in the current API.
There was a problem hiding this comment.
This is definitely a defect/inconsistency in R. Instead of returning NULL (which might trigger a segfault in current or future code), I think Rf_error() is a better choice.
|
I've added some simple tests. |
src/s2-altrep.cpp
Outdated
| } | ||
|
|
||
| static void* s2_altrep_Dataptr(SEXP obj, Rboolean writable) { | ||
| if (writable) return NULL; |
There was a problem hiding this comment.
This is definitely a defect/inconsistency in R. Instead of returning NULL (which might trigger a segfault in current or future code), I think Rf_error() is a better choice.
tests/testthat/test-s2-geography.R
Outdated
| return (double) ((uintptr_t) DATAPTR_RO(obj)); | ||
| }") | ||
|
|
||
| expect_no_error(get_dataptr(as_s2_geography("POINT (0 0)"))) |
There was a problem hiding this comment.
| expect_no_error(get_dataptr(as_s2_geography("POINT (0 0)"))) | |
| expect_true(get_dataptr(as_s2_geography("POINT (0 0)")) != 0) |
There was a problem hiding this comment.
I've changed the test function result to boolean directly as this is more portable
When processing large amounts on data using the
parallelpackage, the session occasionally errors out complaining about missing Dataptr method. It is not easily reproducible, and happens during the collection phase. This patch adds the missing methods.