joshuaulrich / xts

Extensible time series class that provides uniform handling of many R time series classes by extending zoo.
http://joshuaulrich.github.io/xts/
GNU General Public License v2.0
220 stars 71 forks source link

Correct .Call() function signatures #317

Closed joshuaulrich closed 4 years ago

joshuaulrich commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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

Haven't looked at xts yet.

Edited again, adding an important not above

kalibera commented 4 years ago

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

joshuaulrich commented 4 years ago

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.