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

[eddelbuettel]

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]

I just rebased this off the updated main branch.

[joshuaulrich]

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:

• xtsAPI.h: No such file or directory
• how to grow XTS series in R dynamically ? And Quickly!

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]

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]

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]

Offline, Dirk pointed out that the API broke for do_merge_xts() in 81df3f0745e7d51ae76bb1dd690301e246d76ada 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]

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.