-
Notifications
You must be signed in to change notification settings - Fork 970
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
Remove non-API calls in C #6180
Comments
there is a web page which lists all of the API calls, grouped into 3 categories |
You should probably be proactive and contact R-core regarding your need of an API to |
if this is important for you (or anyone else reading) please take the time to construct a performance comparison. |
I would but unfortunately I think you need at least basic knowledge of C to do that. |
AFAIU we can replace |
Where is |
We use it as part of memrecycle and subsetting so its basically in most operations :D But we are not alone with this problem r-lib/vctrs#1933 |
Sure, but I'm wondering what the tests will be focused on (since this affects multiple functions) and then what about |
I don't see By the same definition, touching S4 objects from C is also non-API! 🤔 |
I see, a = 1:10
address(a)
# [1] "0x5651b8564b30"
length(a) <- 20
address(a)
# [1] "0x5651b8ab5c68" |
It looks like more of our usage has been marked as non-API in the meantime:
new:
Lines 30 to 32 in fab93a9
Line 20 in fab93a9
Lines 58 to 63 in fab93a9
Lines 548 to 552 in fab93a9
Lines 10 to 12 in fab93a9
Line 535 in fab93a9
|
there is a new section in WRE https://cran.r-project.org/doc/manuals/r-devel/R-exts.html#Moving-into-C-API-compliance |
@tdhock there is nothing about |
I can imagine group by, when not gforce optimized, is setting true length as well. It allocates memory for a biggest group and then reuses that space for smaller groups as well. |
This issue also makes our CI fail since we get now a new 3rd NOTE: |
Yes, for the moment till we address this issue. CI is used to spot new issues, this one is already known. |
We are getting these NOTEs on CRAN, e.g.
Found non-API calls to R: ‘SETLENGTH’, ‘SET_S4_OBJECT’,
‘SET_TRUELENGTH’, ‘UNSET_S4_OBJECT’
I'm really skeptical about the SETLENGTH and SET_TRUELENGTH parts, which have long been a key underlying feature of data.table internals. I propose we try and keep using those unless CRAN really forces our hands. It be really great if someone wanted to do a performance comparison of the package with/without those calls.
Anyway, it should be easier to remove the non-API S4 calls.
The list does keep changing as this is an actively evolving piece of r-devel. Here's the current list of things we should (eventually) address:
The text was updated successfully, but these errors were encountered: