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

Update API header and standardize two signatures (closes #317) #337

Merged
merged 4 commits into from
Jul 22, 2020
Merged

Update API header and standardize two signatures (closes #317) #337

merged 4 commits into from
Jul 22, 2020

Conversation

eddelbuettel
Copy link
Contributor

This pull request start as a very narrow fix for the obvious bug of xtsAPI.h not having caught up with the expanded signature of xtsMerge -- the more recently added parameter was not reflected. That is the first commit, and I would be entirely open to cherry picking it out and making this a separate PR.

The remainder addresses the longer overdue issue #317 and corrects two signatures called by .Call to be all SEXP as required by the R API contract. The changes may look invasive but they really are as the fix "merely" consists of replacing (scalar) use of an int on the way in and out, respectively, for do_merge_xts and is_xts -- so it really only consisted of adding the appropriate R API wrapper for to/from integer conversion.

Feel free to merge or ignore or squash or whatever. It would be terrific the exposed xtsAPI.h header could make its way to CRAN. As it is currently stands I cannot call xtsMerge from the RcppXts package due to the mismatch.

@eddelbuettel eddelbuettel mentioned this pull request Jul 19, 2020
@eddelbuettel
Copy link
Contributor Author

I just rebased this off the updated main branch.

@joshuaulrich
Copy link
Owner

joshuaulrich commented Jul 22, 2020

Thanks for taking a look at this, and for the PR. I had started working on the issue too, but your changes made it clear to me that my approach was wrong.

I had thought it mattered that isXts() and do_merge_xts() were never called in xts R code, even though they were registered as callable via .Call(). I incorrectly thought I could unregister those two functions to fix the issue Tomas reported. I now understand that would make it impossible for RcppXts to call them.

So we have to break the current API to fix the issues. It's hard to know who's using the API, other than RcppXts. But Google says there are some people out there who are using RcppXts:

My thoughts on best practices to make breaking changes to the API:

  • add a XTS_API_VERSION to xtsAPI.h
  • add __attribute__((deprecated)) to int xtsIs(SEXP) and SEXP xtsMerge(SEXP, ..., int) if the API version is < 2.
  • if the API version is >= 2, declare and define SEXP xtsIs(SEXP) and SEXP xtsMerge(SEXP, ..., SEXP) and handle the conversions in the definitions

Here's my stab at it:

diff --git a/inst/include/xtsAPI.h b/inst/include/xtsAPI.h
index a088c76..83b87ee 100644
--- a/inst/include/xtsAPI.h
+++ b/inst/include/xtsAPI.h
@@ -16,6 +16,10 @@ as the full xts software, GPL (>= 2).
 #ifndef _XTS_API_H
 #define _XTS_API_H
 
+#ifndef XTS_API_VERSION
+#define XTS_API_VERSION 1
+#endif
+
 #include <xts.h>		// also includes R.h, Rinternals.h, Rdefines.h
 
 #include <Rconfig.h>
@@ -41,12 +45,22 @@ extern "C" {
     fun = ( RETURNTYPE(*)(ARG1,ARG2)) R_GetCCallable("PACKAGENAME", "FUNCTIONNAME")
 
 */
-int attribute_hidden xtsIs(SEXP x)
+#if XTS_API_VERSION < 2
+int attribute_hidden __attribute__((deprecated)) xtsIs(SEXP x)
 {
   static int(*fun)(SEXP) = NULL;
   if (fun == NULL) fun = (int(*)(SEXP)) R_GetCCallable("xts","isXts");
   return fun(x);
 }
+#else
+SEXP xtsIs(SEXP);
+SEXP attribute_hidden xtsIs(SEXP x)
+{
+  static int(*fun)(SEXP) = NULL;
+  if (fun == NULL) fun = (int(*)(SEXP)) R_GetCCallable("xts","isXts");
+  return Rf_ScalarLogical(fun(x));
+}
+#endif
 
 SEXP attribute_hidden xtsIsOrdered(SEXP x, SEXP increasing, SEXP strictly)
 {
@@ -104,13 +118,25 @@ SEXP attribute_hidden xtsEndpoints(SEXP x, SEXP on, SEXP k, SEXP addlast) {
     return fun(x, on, k, addlast);
 }
 
-SEXP attribute_hidden xtsMerge(SEXP x, SEXP y, SEXP all, SEXP fill, SEXP retclass, 
+#if XTS_API_VERSION < 2
+SEXP attribute_hidden __attribute__((deprecated)) xtsMerge(SEXP x, SEXP y, SEXP all, SEXP fill, SEXP retclass, 
                                SEXP colnames, SEXP suffixes, SEXP retside, SEXP env, int coerce) {
     static SEXP(*fun)(SEXP,SEXP,SEXP,SEXP,SEXP,SEXP,SEXP,SEXP,SEXP,int) = NULL;
     if (fun == NULL) 
         fun = (SEXP(*)(SEXP,SEXP,SEXP,SEXP,SEXP,SEXP,SEXP,SEXP,SEXP,int)) R_GetCCallable("xts","do_merge_xts");
     return fun(x, y, all, fill, retclass, colnames, suffixes, retside, env, coerce);
 }
+#else
+SEXP xtsMerge(SEXP, SEXP, SEXP, SEXP, SEXP, SEXP, SEXP, SEXP, SEXP, SEXP);
+
+SEXP attribute_hidden xtsMerge(SEXP x, SEXP y, SEXP all, SEXP fill, SEXP retclass, 
+                               SEXP colnames, SEXP suffixes, SEXP retside, SEXP env, SEXP coerce) {
+    static SEXP(*fun)(SEXP,SEXP,SEXP,SEXP,SEXP,SEXP,SEXP,SEXP,SEXP,SEXP) = NULL;
+    if (fun == NULL) 
+        fun = (SEXP(*)(SEXP,SEXP,SEXP,SEXP,SEXP,SEXP,SEXP,SEXP,SEXP,SEXP)) R_GetCCallable("xts","do_merge_xts");
+    return fun(x, y, all, fill, retclass, colnames, suffixes, retside, env, coerce);
+}
+#endif
 
 SEXP attribute_hidden xtsNaOmit(SEXP x) {
     static SEXP(*fun)(SEXP) = NULL;

Even if we accept this PR as-is, I still think it would be good to give potential users a warning before forcing the breaking changes.

Thoughts?

@eddelbuettel
Copy link
Contributor Author

eddelbuettel commented Jul 22, 2020

Do whatever you need to do, close this, what have you.

RcppXts is currently borked. I worked on this to

  1. unbork it
  2. help you with an open and seven month old bug in your package
  3. if it doesn't help so be it
  4. RcppXts is still on CRAN and borked (cannot call xtsMerge due to broken signature in xtsAPI.h)

Rank order as you see fit, a CRAN update to the header file would be appreciated.

Edit: Deprecation is ex ante a good idea. I flipped the bit, so you can commit into this fork and PR to tune it if you so choose.

@joshuaulrich
Copy link
Owner

Sorry that the original version of the now-edited second paragraph made it sound like some of these changes weren't necessary. I thought that was true when I started writing the comment, but realized it was wrong as I looking at the code while writing my response. I updated it to make it clear that I was wrong, but I left my thought process there.

@joshuaulrich
Copy link
Owner

Offline, Dirk pointed out that the API broke for do_merge_xts() in 81df3f0 which is in version 0.12.0 and has been on CRAN for > 1 year. I haven't had any reports of issues (beyond Tomas'), so I don't think deprecating is necessary even though it would usually be best practice.

@joshuaulrich joshuaulrich merged commit 6287228 into joshuaulrich:master Jul 22, 2020
@joshuaulrich
Copy link
Owner

Thanks again for this PR, and your patience as I slowly came to realize my errors. I'll work on a release and hopefully be able to get one out this weekend.

@eddelbuettel eddelbuettel deleted the feature/update_api_header branch July 30, 2020 02:37
@joshuaulrich joshuaulrich added this to the 0.12-1 milestone Aug 2, 2020
@joshuaulrich joshuaulrich linked an issue Aug 2, 2020 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.

Correct .Call() function signatures
2 participants