-
Notifications
You must be signed in to change notification settings - Fork 36
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
Explicitly set locale for some test files #603
Conversation
Thanks @MichaelChirico! Wondering what brings me the honor of you making these edits to collapse's testing facilities? Also wondered, did CRAN force you to do something about
? I have some of the same notes now. |
Finally got around to importing {collapse} as a third-party package internally. We have a "unique" testing environment different from CRAN which can dredge up these sorts of edge cases.
We have not submitted to CRAN yet, not sure what will be enforced. Follow along at Rdatatable/data.table#6180 :) |
Ok, thanks! And cool that collapse can now be used at Google :) |
Probably not the way you'll go about it @MichaelChirico, but for the radixsort I implemented a custom TRUELENGTH/SET_TRUELENGTH, which avoids an ALTREP check that made the function noticeably more expensive: https://github.com/SebKrantz/collapse/blob/master/src/base_radixsort.h. |
Thanks for sharing! I'd still like to strive to use the public API if possible, let's see how it goes :) |
@MichaelChirico further to this, I've now done things the dirty way again. CRAN was not responding anymore to my submissions, which I guess is now the way they start cracking down on all non-API compliant smaller packages (not helpful). Just sharing in case it is useful (I can see that the ALTREP solution of aitap (Ivan) is cool but would of course overhaul a large part of your architecture, don't allow #define STDVEC_TRUELENGTH(x) (((VECSEXP) (x))->vecsxp.truelength)
// No method to set ALTREP_TRUELENGTH (gives error)
#define SET_TRULEN(x, v) (STDVEC_TRUELENGTH(x)=(v))
// ALTREP_TRUELENGTH is 0: https://github.com/wch/r-source/blob/48f06c1071fea6a6e7e365ad3d745217268e2175/src/main/altrep.c#L345
#define TRULEN(x) (ALTREP(x) ? 0 : STDVEC_TRUELENGTH(x))
|
I was seeing naming mismatch errors without setting locale to C here.