-
Notifications
You must be signed in to change notification settings - Fork 65
Phase in the new resizable vector API #582
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
Conversation
|
@DavisVaughan I don't think there's anything that interesting to see here, just recruiting a fellow human. |
|
I guess there is some question about where to put the backport. It's not really about altrep but that's where I stuck it. |
| cpp11::writable::list res(num_cols + add_filename); | ||
| cpp11::writable::list res( | ||
| R_allocResizableVector(VECSXP, num_cols + add_filename)); | ||
|
|
||
| cpp11::writable::strings res_nms(num_cols + add_filename); | ||
| cpp11::writable::strings res_nms( | ||
| R_allocResizableVector(STRSXP, num_cols + add_filename)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am honestly quite nervous about what putting these in a cpp11::writable::list or cpp11::writable::strings will do. It...probably...works fine, but this is so experimental it is hard to know.
Would it be possible to put them in a cpp11::sexp instead? I would feel more comfortable with that even though it will require more code changes below, because you'll need SET_VECTOR_ELT() and friends.
|
The main thing I am worried about is the fact that this is still so up in the air. Luke can change it at any time right now, forcing another release on you. |
|
@DavisVaughan Will you take a quick glance just to make sure that I interpreted your feedback correctly? I also decided to move the (updated) backports into |
Closes #561
This is the last move to get into C API compliance 🎉