Changes to address upcoming problem of #447 and USE_FC_LEN_T

[gavinsimpson]

This (is a work-in-progress) PR that addresses the issue reported by CRAN with regard to calls to fortran code that involves strings. @jarioksa noted that we do this in src/getF.c with calls to LAPACK functions.

The PR adds the required code that R itself uses to handle the problem, exploiting the ability to pass string lengths as hidden variables to the compiler.

Just checking that this works on an older version of R (currently checking with 3.5)

[gavinsimpson]

@jarioksa This all seems to be working now on an old version of R without needing to #ifdef whole functions, so this PR is no longer a work-in-progress and can be reviewed whenever time allows

[jarioksa]

Yep, this is similar as I did, but never wanted to push to vegan. There was several years ago a plan to change Fortran call in GNU fortran, but it never was done, but you can think that it may be implemented in some future version of GNU fortran and therefore R calls must be changed. Even if this would happen, it would concern cases where character string constant is used as an argument to the call, but we do not use character string constant: we use a character[2] variable. To be consistent with this scenario, the call should be changed from F77_CALL(dgesdd)(jobz, ...) to F77_CALL(dgesdd)("N",... FCONE) (etc) to first create and then avoid potential problems in the future. I think this was a problem with Fortran when I wrote src/getF.c because I wrote it like I wrote: used char[2] jobz = "N"in the call instead of literal constant "N". I think I did it because that natural way did not work. However, it is so long time that I don't remember certainly my reasons for the unnatural construction, and I didn't push stages of my work that failed to compile to a correct code.

[jarioksa]

I had a closer look at this and preprocessed the source file (switch -E in gcc and clang). LAPack dgesdd (for SVD) call looks after pre-processing:

/* Definition in the header */
dgesdd_(const char* jobz,
   const int *m, const int *n,
   double *a, const int *lda, double *s,
   double *u, const int *ldu,
   double *vt, const int *ldvt,
   double *work, const int *lwork, int *iwork, int *info ,size_t );

/* Call in svdfirst() in getF.c */
    dgesdd_(jobz, &nr, &nc, xwork, &nr, sigma, &dummy,
       &nr, &dummy, &nc, &query, &lwork, iwork, &info ,(size_t)1);

The switches add ,(size_t)1 as the last argument in the call. This last size_t will be only there after #define USE_FC_LEN_T . Now there is a small inconsistency, but I'm not sure if it matters (or even if this change really matters). To avoid problems back in 2016 I didn't use character constant "N" in the call, but I had

char[2] jobz = "N";

That is, we pass a variable of length 2 (it must be of length 2 in C, because character strings are terminated with NULL, but (size_t)1 defines a character of length 1. I guess the terminating NULL is just ignored. However, the new interface allows passing literals, and instead of jobz we could just use "N" in the call. That is

    F77_CALL(dgesdd)("N", &nr, &nc, xwork, &nr, sigma, &dummy,
                     &nr, &dummy, &nc, &query, &lwork, iwork, &info FCONE);

which may be more readable.

In eigenfirst we have three character strings, jobz, range, uplo that could be made into "N", "I", "L" which may or may not be more readable.

I find the the new call interface really confusing: you just add FCONE FCONE FCONE which looks like a syntax error, but preprocessor adds the "missing" comma as FCONE seems to expand to ,(size_t)1.

[jarioksa]

About backward compatibility: if USE_FC_LEN_T is undefined (such as in R < 3.6.2?), FCONE will just expand to nothing, and in effect swiped out. This leaves the call like it was, that is without size_t in definition, and without ,(size_t)1 in the call, or like it was when it was written. This should work at least with the current char[2] jobz = "N"; idiom, as the WRExt 6.6.1 writes:

For earlier versions of R we could just assume that msg is nul-terminated (not guaranteed, but people have been getting away with it for many years)

and our practice of defining jobz etc. variables guarantees that input strings are NULL terminated. So perhaps we leave the code like it is now; that is, we do not change the calls to "N", or "N", "I", "L", but use the safe pedestrian approach i was forced to use back in 2016.

[jarioksa]

Please see the comments in "Conversation"

[gavinsimpson]

Thanks Jari; have read and followed you comments. I agree with your conclusion to just leave the code alone and call with jobz etc rather string literals. Using FCONE is what R Core recommend and what R does, so unless anyone has a burning desire to see string literals in the code calls let's leave them alone and just make the minimal set of changes here. We can always revisit moving to string literals at some later date if someone wants to revisit it, but in anticipation of R Core suddenly deciding we didn't do something they told us about some time ago I'm going to merge this to the master branch.