ralna / spral

Sparse Parallel Robust Algorithms Library
https://ralna.github.io/spral/
Other
106 stars 26 forks source link

Fix long integer on Windows #96

Closed jfowkes closed 1 year ago

jfowkes commented 1 year ago

Switch from using C long to C int64_t so that long integers on Windows are always 64bit.

Long integers on the Fortran side are always 64bit as they are specified with selected_int_kind(18)

jfowkes commented 1 year ago

@amontoison how does this look? Could you test on Windows?

jfowkes commented 1 year ago

@mjacobse what are your thoughts on this issue? I think int64_t should be preferred to long long as it is exactly 64bit.

mjacobse commented 1 year ago

First of all I very much agree with replacing long in the C interfaces with something that is 64 bit on Windows too.

I believe some of the C interfaces will not work with the current changes though. Some of the functions are renamed in the C headers (ending in _long to ending in _int64_t), but as far as I can tell not in the Fortran implementations that are tied to those C definitions with bind(C). I believe this will result in linker errors for anyone who tries to use these functions from C. This could be fixed by either renaming the corresponding Fortran functions in the same way, or by explicitly giving the new name of the C functions to the optional name parameter of the BIND(C) attribute.

Perhaps you already considered this, but another thought I had was that users of those renamed functions will have their existing code break due to the API change when upgrading to a new version. Granted, the required changes in their code should be pretty simple. But if the function names stayed the same, at least the code for most users on Linux should continue to work correctly just like before. So that's a consideration of changing to a more appropriate name vs. not breaking some users' code.

As for long long vs. int64_t I don't really have a strong opinion. In practice I think it should come down to the same thing on most systems. You already mentioned the advantage of int64_t being exactly 64 bit, so for the sake of argument a few (pretty weak) advantages of long long that I can think of:

jfowkes commented 1 year ago

Thanks @mjacobse, apologies I mistakenly renamed some of the functions that were ending in _long which was not intended, this has now been fixed. I think for backwards compatibility it's probably better to keep the old names as you suggest.

Also thank you for listing the advantages of long long, as you say these seem quite weak so I'm tempted to go with int64_t instead, especially as it seems to have greater adoption in this context, e.g. as @amontoison points out int64_t is what they use in SuiteSparse now: https://github.com/DrTimothyAldenDavis/SuiteSparse/pull/134#discussion_r977195923