Skip to content

Conversation

@echoix
Copy link
Member

@echoix echoix commented Aug 3, 2025

These are similar to the changes made in the upstream repo in OSGeo/grass@d5bb442, where CBLAS was used and ATLAS dropped.

Before that commit, doublereal was typedef-ed to double, if GCC's g2c (GCC's version of f2c) was not present, in include/grass/la.h

These are similar to the changes made in the upstream repo in OSGeo/grass@d5bb442, where CBLAS was used and ATLAS dropped.

Before that commit, doublereal was typedef-ed to double, if GCC's g2c (GCC's version of f2c) was not present.
@echoix echoix requested a review from nilason August 3, 2025 20:44
@nilason
Copy link
Contributor

nilason commented Aug 4, 2025

It is only GRASS 8.5+ that uses the CBLAS interface. This code needs to be GRASS version aware, off the head I'm not sure what would work for Addons.

In principle something like:

#ifdef HAVE_CBLAS_H
// new code
#else
// old code
#endif

@echoix
Copy link
Member Author

echoix commented Aug 4, 2025

What about OSGeo/grass@d5bb442#diff-1df0e75a8f47991f0c38f6ff94dadf2f0d2203321bf6346136360b099ff0b086L40, where HAVE_G2C_H is used. Could #ifndef HAVE_G2C_H be appropriate?

Thinking about it again, it’s been a while since we are using gcc 4+, so we are already in the fallback where ˋtypedef double doublereal;` is used. With LLVM/clang, I suppose we were always using this.
So, is the fix of this PR really limited to GRASS 8.5+? If so, wouldn’t addons benefit of having more granular branches?

@nilason
Copy link
Contributor

nilason commented Aug 5, 2025

What about OSGeo/grass@d5bb442#diff-1df0e75a8f47991f0c38f6ff94dadf2f0d2203321bf6346136360b099ff0b086L40, where HAVE_G2C_H is used. Could #ifndef HAVE_G2C_H be appropriate?

I realised now that HAVE_CBLAS_H may have already be set also before 8.5 (not reflecting current changes in GRASS), so the version check I originally had in mind is more appropriate:

#include <grass/version.h>

#if GRASS_VERSION_MINOR >= 5
// new code
#else
// old code
#endif

Thinking about it again, it’s been a while since we are using gcc 4+, so we are already in the fallback where ˋtypedef double doublereal;` is used. With LLVM/clang, I suppose we were always using this.

If there was an issue with the previous code (pre-OSGeo/grass@d5bb442), that should be addressed separately.

So, is the fix of this PR really limited to GRASS 8.5+? If so, wouldn’t addons benefit of having more granular branches?

It would be a pain to maintain each of a minor version addos branch (if you mean a grass85 branch etc), so I'd think not.

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.

2 participants