Skip to content
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

Improvements to C code, fix #360 #361

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

ArcadeAntics
Copy link

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
Copy link
Owner

klmr commented May 8, 2024

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
Copy link
Author

ArcadeAntics commented May 9, 2024

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

@klmr klmr self-assigned this Aug 4, 2024
@klmr klmr linked an issue Aug 4, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected error when trying to access non-existent variable from module
2 participants