GraphBLAS: Correctly identify `clang-cl` as MSVC-compatible compiler

[mmuetzel]

For clang-cl, both _MSC_VER and __clang__ are set. Move check for _MSC_VER to before check for __clang__ to allow identifying clang-cl as a MSVC compatible compiler.

This should be fixing #751.

[mmuetzel]

The second commit adds feature tests that are run during configuration to determine how complex numbers need to be handled in C. Rather than deciding based on the compiler vendor, the result of that check is used in the respective preprocessor conditions.

[DrTimothyAldenDavis]

I'm slowly getting up to speed after a nasty cold, so my replies will be sluggish.

Checking at compile time for how complex types are handled might be necessary, but there can be drawbacks. What if one compiler is used to compile GraphBLAS and create GraphBLAS.h, but another compiler is used to build another application that uses GraphBLAS.h?

I already have that potential issue in SuiteSparse_config, where I detect the Fortran-to-C interface mechanism. The cmake configuration handles the typical case. Figuring out how C calls Fortran is tricky, and fortunately most compilers on the same OS seem to make the same choices as well. However, since the user's compiler might differ from the one used to configure SuiteSparse_config.h, I have a fallback mechanism where the user can pass in -DBLAS_NO_UNDERSCORE(for example). The same is true for -DBLAS64 in SuiteSparse_config.h.

If there's something OS-dependent, or for something that all compilers on a given OS treat the same way, this issue doesn't arise. For something that's compiler-dependent, it's safest and most portable if a compiler can figure out how to do things itself by processing a *.h header file, without the help of cmake configure step (like #ifdef _MSC_VER in the current GraphBLAS.h). Failing that, it would be important to have some kind of fallback where the user can specify at their compile time (not mine) how complex types are handled.

Alternatively, what if something like this could work. Rather than starting with an #if test as I do now, first #include the complex.h header:

#include <complex.h> then check if a proper #define appears in that complex.h. For example. the C11 spec says that complex is a macro defined by complex.h that expands to _Complex:

If the MSVC complex.h does not #define this macro, then the #if / #else I currently use could be replaced with

#ifndef complex
...  current code for Microsoft complex types
#else
...  current case for C11 complex types
#endif

Looking at https://learn.microsoft.com/en-us/cpp/c-runtime-library/complex-math-support?view=msvc-170#complex-constants-and-macros it seems that the MSVC complex.h file would not #define complex:

The Microsoft C Runtime library (CRT) provides complex math library functions, including all of the ones required by ISO C99. The compiler doesn't directly support a complex or _Complex keyword, therefore ...

If that works, it would be more portable and allow one compiler to be used by cmake to configure GraphBLAS.h, and another compiler in a user application that #include's GraphBLAS.h.

[mmuetzel]

Checking at compile time for how complex types are handled might be necessary, but there can be drawbacks. What if one compiler is used to compile GraphBLAS and create GraphBLAS.h, but another compiler is used to build another application that uses GraphBLAS.h?

If that works, it would be more portable and allow one compiler to be used by cmake to configure GraphBLAS.h, and another compiler in a user application that #include's GraphBLAS.h.

As long as there are no complex number types in the API, that shouldn't be an issue. If the API uses complex number types, users need to use compatible toolchains or the ABI would be incompatible. There is no way around that. (Nothing that SuiteSparse can do.) How complex numbers are handled is part of the C ABI. There is no need for a user to be able to override that (because trying to do that is guaranteed to break things).

Alternatively, what if something like this could work. Rather than starting with an #if test as I do now, first #include the complex.h header:

include

then check if a proper #define appears in that complex.h. For example. the C11 spec says that complex is a macro defined by complex.h that expands to _Complex:

It might be personal preference. But in my opinion, a feature test is always better than inferring some property by some other property. E.g., MSVC might not define complex in their current version. But afaict there is no guarantee that they'll never define it. Or another header that is included before GraphBLAS.h in a user project happens to define it.

On the other hand, there might be a future compiler that does not define _MSC_VER but that might still use the MSVC C ABI when it comes to complex numbers...

[DrTimothyAldenDavis]

If the API uses complex number types, users need to use compatible toolchains or the ABI would be incompatible...

Good point. So letting cmake handle it would probably be ok. I'll take another look with that in mind when I get back to the keyboard.

[DrTimothyAldenDavis]

Overall I think the strategy is fine -- thanks for the update. I have some minor revisions to make but I think it might be easier if I just made them myself after merging it in. One is an minor change (line 890 of GB_ops_template.h), but others are harder to explain in a merge review.

[DrTimothyAldenDavis]

I made my revisions here: https://github.com/DrTimothyAldenDavis/SuiteSparse/commit/881e4844357f58064ceaae5f481d86a1751ca1e3

A few highlights: GxB_complex.h is no longer needed. It's a holdover from an earlier version of the JIT. So I removed it to avoid the code duplication. I also don't need GxB_config.h as a result.

I renamed the new GXB_HAVE macros to GxB_HAVE, to be consistent with my prior GxB_* macros in GraphBLAS.h.