Convert readxl from Rcpp to cpp11 #659
Conversation
|
I had to edit a few other files to define the macros
It was easier then try to figure out how to reorganize the header files. |
There were a few cases where as far as I could tell you were converting an R SEXP to another R SEXP using as_cpp(), which you should never need to do.
The mental model is as_cpp() is used to convert a SEXP to a native C++ type (like std::string, int, etc.) and as_sexp() is used to go the opposite way, e.g. convert a C++ type to an R SEXP.
But in general you should have to call these explicitly somewhat rarely, most of the time they can be done implicitly in the generated registration code.
src/XlsxWorkSheet.h
Outdated
| @@ -198,7 +204,7 @@ class XlsxWorkSheet { | |||
|
|
|||
| xcell->inferType(na, trimWs, wb_.stringTable(), dateFormats_); | |||
| CellType type = xcell->type(); | |||
| Rcpp::RObject column = cols[col]; | |||
| cpp11::sexp column = cpp11::as_sexp(cols[col]); | |||
There was a problem hiding this comment.
Do we need this call to as_sexp()? I would guess no? maybe just this?
| cpp11::sexp column = cpp11::as_sexp(cols[col]); | |
| cpp11::sexp column(cols[col]); |
There was a problem hiding this comment.
I ended up having to do something similar to what we did with
std::vector<ColType> colTypes = colTypeStrings(static_cast<SEXP>(col_types));
Because I was getting a constructor error but let me know if there is another solution!
src/XlsxWorkSheet.cpp
Outdated
|
|
||
| // catches empty sheets and sheets where requested rectangle contains no data | ||
| if (ws.nrow() == 0 && ws.ncol() == 0) { | ||
| return Rcpp::List(0); | ||
| cpp11::list x; |
There was a problem hiding this comment.
This would need to be a writable list I think? Otherwise it is going to return NULL instead of list()
| cpp11::list x; | |
| cpp11::writable::list(); |
src/XlsxWorkSheet.cpp
Outdated
| bool has_col_names = false; | ||
| switch(TYPEOF(col_names)) { | ||
| case STRSXP: | ||
| colNames = as<CharacterVector>(col_names); | ||
| colNames = cpp11::as_cpp<cpp11::strings>(col_names); |
There was a problem hiding this comment.
I don't think you need any cast? just colNames = col_names
| @@ -47,7 +48,7 @@ List read_xlsx_(std::string path, int sheet_i, | |||
| if (TYPEOF(col_types) != STRSXP) { | |||
| Rcpp::stop("`col_types` must be a character vector"); | |||
| } | |||
| std::vector<ColType> colTypes = colTypeStrings(as<CharacterVector>(col_types)); | |||
| std::vector<ColType> colTypes = colTypeStrings(cpp11::as_cpp<cpp11::strings>(col_types)); | |||
There was a problem hiding this comment.
Not sure you need a cast here either
This cleans up the call a bit, also not using brace initialization avoids ambigious call during compilation using gcc
| #include <set> | ||
| #include <vector> |
There was a problem hiding this comment.
Does it make more sense to keep the system includes, e.g. <whatever> together and above the ones defined here, e.g. "utils.h"?
If you agree, that would be a good thing to check globally. I think that is (mostly?) how things started.
There was a problem hiding this comment.
Yes! Thank you for reminding me about this.
I rearranged some of the header files to avoid conflicts with Rcpp and cpp11 libraries so I need to reorganize them (or organize them back to how you had them). I ended up using the macros #define R_NO_REMAP and #define STRICT_R_HEADERS instead to avoid the conflicts, which I'll remove since they won't be needed anymore once this PR is closed.
There was a problem hiding this comment.
The current state seems to be much more regular, but often puts the local headers before the system headers. It feels like a more conventional order would be: system, vendored code (libxls), written-by-us, which also reflects intended precedence. (Let me also acknowledge that this probably doesn't matter and is more a matter of style at this point.)
There was a problem hiding this comment.
Ah yes sorry! I should have reread your comment before executing based on my memory. I'll rearrange in that order.
We need to remove the rest of the Rcpp references as well. You can find them with find my files or with git grep Rcpp
- Remove Rcpp references from Imports and LinkingTo
- Remove the importFrom Rcpp roxygen annotation in R/readxl.R (and run devtools::document() afterwards)
- Remove any stray
#include "Rcpp.h"directives
You may find some compilation errors, as Rcpp includes a number of standard headers which cpp11 may not. If any of these (like std::vector or std::map etc.) are used in the generated cpp11.cpp code you will need to include them in a special file, src/readxl_types.h which cpp11 will automatically include in the generated file.
|
It looks like the only test still failing is on windows release due to |
I think the Rcpp::stop crash is happening because unwind protect does a long jump, and long jumping from a constructor is AFAIK undefined behavior. I am not clear about the cpp11::warning case, but possibly it could be a similar issue.
cpp11 automatically translates strings to UTF-8 when you construct a std::string. Rcpp did not do this, and the existing tests assumed they would be converted in the native encoding. so we do this explicitly using Rf_translateChar()
|
|
|
Woohoo! |
This initial commit contains the first steps for converting from Rcpp to cpp11
Now I'll convert file by file and push each time I've completed a file so we can review.