search

klmr / box

Write reusable, composable and modular R code
https://klmr.me/box/
MIT License
829 stars 47 forks source link

Improvements to C code, fix #360 #361

Open ArcadeAntics opened 2 months ago

ArcadeAntics commented 2 months ago

This fixes the issue #360 which assumes the first argument must be a symbol. It it is not a symbol, it will also check attr(e1, "name") and format.default(e1) when making an error message.

This also uses .External2(c_strict_extract instead of .Call(c_strict_extract and removes some no longer needed functions.

Also, just some miscellaneous fixes to the C code.

klmr commented 2 months ago

Hi @ArcadeAntics, sorry for the radio silence I’ve had a few rather busy weeks.

Unfortunately we have a problem: there’s official confirmation that .External2() is not part of R’s API, and must not be used.

We could switch over to using .External() but I’d like to measure the performance impact of that before committing to it, since it needs to call environment() unconditionally in the R code.

Otherwise the PR looks good — I’ll make a few changes to adapt the code style and remove obsolete comments, but those are minutae.

ArcadeAntics commented 1 month ago

Hiya @klmr,

Yes, I'd seen that response from Luke last week. Disappointing, but I'll probably continue using it in my own package until they officially revoke its use. I don't think they will anytime soon since it's used in some popular packages like magrittr, vctrs, and rlang. Regardless, I completely understand avoiding using it.

From what I remember, the performance of .External is 100 nanoseconds slower than .Call. That being said, I still prefer .External in my own code because that means my C functions can take a variable number of arguments. Even for my C functions which do not take a variable number of arguments, I still use .External to keep the C functions all looking the same.

Also, very recently I saw there are more functions outside of R_NO_REMAP that nonetheless have remapped names. Those are PROTECT remapped to Rf_protect and UNPROTECT to Rf_unprotect, seen here: https://github.com/wch/r-source/blob/387f2e8728e47dab2fe2c9aabbeb295f687a2272/src/include/Rinternals.h#L371