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

cpp_register(): use_package by default #230

Closed
wants to merge 1 commit into from
Closed

Conversation

alyst
Copy link
Contributor

@alyst alyst commented Aug 11, 2021

For some reason the current symbol-based approach didn't work for me (R 4.0.5) resulting in Symbol not found errors for .Call().

There is already support for an alternative .Call(..., PACKAGE='...') approach, I just extended it to cpp_register() function and made it the default.

@jimhester
Copy link
Member

Thanks for working on this, but I am confident that the symbols will work regardless of the R version and we use symbols for packages for a reason.

I suspect there is something else going wrong with registration in your package, would it be possible for you to provide a link to it?

@alyst
Copy link
Contributor Author

alyst commented Aug 19, 2021

@jimhester It could be because I've missed .registration = TRUE in my useDynLib() directive. Now the symbol-based wrappers seem to work.

For this PR I can set the cpp_register(..., use_package = FALSE, ...), so that the cpp11 behavior would not change,
but still there would be a more complete support for generating the wrappers that explicitly specify the package.

@jimhester
Copy link
Member

Sure we can expose the argument, though in general I don't think most packages would ever need to use it.

Also FWIW if you use usethis::use_cpp11() to setup cpp11 for your package it will add a roxygen annotation to include .registration = TRUE in your namespace directive.

so that the generated wrapper code calls C++ functions as
.Call("func", ..., PACKAGE="pkg")
@alyst
Copy link
Contributor Author

alyst commented Aug 19, 2021

Ok, I've changed to use_package=FALSE.

@DavisVaughan
Copy link
Member

I've decided not to do this for now, as symbols work everywhere and are the most useful thing to use

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.

3 participants