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

Correct .Call() function signatures #317

Closed
joshuaulrich opened this issue Dec 9, 2019 · 5 comments · Fixed by #337
Closed

Correct .Call() function signatures #317

joshuaulrich opened this issue Dec 9, 2019 · 5 comments · Fixed by #337
Assignees
Labels
Milestone

Comments

@joshuaulrich
Copy link
Owner

joshuaulrich commented Dec 9, 2019

All C functions called via .Call() must return SEXP and have all SEXP arguments. This is not true for do_merge_xts() and isXts(). This may change the API, and impact RcppXts. Need to confirm.

Via email from @kalibera:

Could you please check the signatures of foreign functions in xts? It
seems that do_merge_xts is taking a non-SEXP argument, but it is called
via .Call interface (all arguments have to be SEXP). Also isXts() is
registered as a .Call foreign function, but it does not return SEXP.
Both of these issues could probably lead to segfaults, so it would be
great if you could fix them soon.

@joshuaulrich joshuaulrich added this to the 0.12-0 milestone Dec 9, 2019
@joshuaulrich joshuaulrich self-assigned this Dec 9, 2019
@joshuaulrich
Copy link
Owner Author

joshuaulrich commented Dec 11, 2019

@eddelbuettel My preliminary checks suggest this is going to affect RcppXts. My understanding is that I need to do one of two things to do_merge_xts() and isXts():

  1. Do not register them as C-callable functions in init.c, or
  2. Change the definitions so both functions only take SEXP arguments and return SEXP (isXts() returns int).

What are your thoughts/preferences?

@kalibera
Copy link

kalibera commented Dec 11, 2019

To clarify, not registering a function will not solve the problem. If a function is not registered, but is called via .Call, it still has to return SEXP and all of its arguments must be SEXP.

@eddelbuettel
Copy link
Contributor

eddelbuettel commented Dec 11, 2019

I got a similar email from Tomas and already fixed it (in digest). Based on that experience, there are two orthogonal issus here:

  • Tomas rightly point out that one should not register for .Call() when the signature is different (doh)
  • One can still register for export and use by another package, that does not require a SEXP signature (I think...) -- see what digest now does in its init.c

Haven't looked at xts yet.

Edited again, adding an important not above

@kalibera
Copy link

kalibera commented Dec 11, 2019

Yes, functions for use with RegisterCCallable/GetCCallable only (not .Call) do not have such restrictions.

@joshuaulrich
Copy link
Owner Author

joshuaulrich commented Dec 27, 2019

Thank you both for your input! It looks like I only need to stop registering those functions for .Call(). Neither of them are called via .Call() in xts:

josh@thinkpad: ~/git/xts/R (master)
> grep -E "\.Call\(.(isXts|do_merge_xts)" *

That also means RcppXts should be okay, since the function signatures do not need to change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants