Update SuiteSparse_config.h

[mmuetzel]

Oof. That looks like a regression from a change last week: https://github.com/DrTimothyAldenDavis/SuiteSparse/commit/341300e6c771c1cfe9508c2de1d24d9d5560f7fe

The proposed change looks good to me in principle. 👍

However, that is a generated file that is overwritten during the build process. Please, also make that change in the file in Config/SuiteSparse_config.h.in.

[mmuetzel]

I'm wondering why the CI didn't trigger that issue.

Which test revealed the issue for you?

[prj-]

PETSc BuildSystem.

[mmuetzel]

PETSc BuildSystem.

Do you know which specific test in PETSc BuildSystem triggered the issue? E.g., which source file did it try to build and failed?

[DrTimothyAldenDavis]

oops. That was a recent change of mine. Not sure how I messed up on that one. I'll release a SuiteSparse 7.5.1 with this fix.

[prj-]

PETSc BuildSystem.

Do you know which specific test in PETSc BuildSystem triggered the issue? E.g., which source file did it try to build and failed?

Any SuiteSparse file, really, as long as you are using the -Wundef compiler flag. PETSc BuildSystem can be picky, and marked the warnings that got generated due to the regression as a failure.

[prj-]

oops. That was a recent change of mine. Not sure how I messed up on that one. I'll release a SuiteSparse 7.5.1 with this fix.

Thanks, it looks like this version will be good to go in PETSc (which is a much needed update since we are currently stuck at 5.13.0).

[prj-]

@mmuetzel, @DrTimothyAldenDavis, you may want to fix these as well.

git.suitesparse/SPQR/Include/spqr.hpp:1118:51: error: unused parameter 'cc' [-Werror,-Wunused-parameter]
inline double spqr_abs (double x, cholmod_common *cc)       // cc is unused
                                                  ^
git.suitesparse/SPQR/Include/spqr.hpp:1123:52: error: unused parameter 'cc' [-Werror,-Wunused-parameter]
inline double spqr_abs (Complex x, cholmod_common *cc)
                                                   ^
git.suitesparse/SPQR/Include/spqr.hpp:1133:64: error: unused parameter 'cc' [-Werror,-Wunused-parameter]
inline double spqr_divide (double a, double b, cholmod_common *cc)  // cc unused
                                                               ^
git.suitesparse/SPQR/Include/spqr.hpp:1138:67: error: unused parameter 'cc' [-Werror,-Wunused-parameter]
inline Complex spqr_divide (Complex a, Complex b, cholmod_common *cc)
                                                                  ^
git.suitesparse/SPQR/Source/SuiteSparseQR.cpp:359:35: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
    RETURN_IF_NULL_COMMON (EMPTY) ;
                                  ^
git.suitesparse/SPQR/Source/SuiteSparseQR.cpp:360:31: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
    RETURN_IF_NULL (A, EMPTY) ;
                              ^
git.suitesparse/SPQR/Source/SuiteSparseQR.cpp:362:40: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
    RETURN_IF_XTYPE_INVALID (A, EMPTY) ;
                                       ^
git.suitesparse/SPQR/Source/SuiteSparseQR.cpp:363:67: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
    if (Bsparse != NULL) RETURN_IF_XTYPE_INVALID (Bsparse, EMPTY) ;
                                                                  ^
git.suitesparse/SPQR/Source/SuiteSparseQR.cpp:364:67: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
    if (Bdense  != NULL) RETURN_IF_XTYPE_INVALID (Bdense,  EMPTY) ;
                                                                  ^
git.suitesparse/SPQR/Source/SuiteSparseQR.cpp:569:18: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
        FREE_ALL ;
                 ^
git.suitesparse/SPQR/Source/SuiteSparseQR.cpp:716:18: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
        FREE_ALL ;
                 ^
git.suitesparse/SPQR/Source/SuiteSparseQR.cpp:850:26: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
                FREE_ALL ;
                         ^
git.suitesparse/SPQR/Source/SuiteSparseQR.cpp:933:22: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
            FREE_ALL ;
                     ^
git.suitesparse/SPQR/Source/SuiteSparseQR.cpp:1017:34: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
                        FREE_ALL ;
                                 ^
git.suitesparse/SPQR/Source/SuiteSparseQR.cpp:1085:22: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
            FREE_ALL ;
                     ^
git.suitesparse/SPQR/Source/SuiteSparseQR.cpp:1146:22: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
            FREE_ALL ;
                     ^
git.suitesparse/SPQR/Source/SuiteSparseQR.cpp:1513:35: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
    RETURN_IF_NULL_COMMON (EMPTY) ;
                                  ^
git.suitesparse/SPQR/Source/SuiteSparseQR.cpp:1514:31: error: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Werror,-Wextra-semi-stmt]
    RETURN_IF_NULL (A, EMPTY) ;

[DrTimothyAldenDavis]

Those are not errors.

The API of CHOLMOD has a last parameter of (Common *) in all functions. Even if it's not used. Removing it in the few cases where it's not used would break the API and make it non uniform. And I might need to use that parameter in the future.

The semicolon is a style issue. Removing it after a macro looks strange so I don't want to remove it.

[prj-]

Removing it

This is not how to fix the issue. The file is named spqr.hpp, which implies it's a C++ file? Parameters in C++ function can have no name (unlike in C), i.e., the fix is inline double spqr_abs (double x, cholmod_common *).

Removing it after a macro looks strange

This is how it's dealt with in PETSc: https://petsc.org/main/include/petsc/private/petschypre.h.html#line50, i.e., you should not have to remove it after the macro usage, just fix the macro definition.

[prj-]

Here is the patch for SuiteSparseQR.

diff --git a/SPQR/Include/spqr.hpp b/SPQR/Include/spqr.hpp
index 1bccda9fe..f44321018 100644
--- a/SPQR/Include/spqr.hpp
+++ b/SPQR/Include/spqr.hpp
@@ -102,3 +102,3 @@
 #define RETURN_IF_NULL(A,result) \
-{ \
+do { \
     if ((A) == NULL) \
@@ -111,3 +111,3 @@
     } \
-}
+} while (0)

@@ -115,3 +115,3 @@
 #define RETURN_IF_NULL_COMMON(result) \
-{ \
+do { \
     if (cc == NULL) \
@@ -120,6 +120,6 @@
     } \
-}
+} while (0)

 #define RETURN_IF_XTYPE_INVALID(A,result) \
-{ \
+do { \
     if (A->xtype != xtype) \
@@ -129,3 +129,3 @@
     } \
-}
+} while (0)

@@ -1117,3 +1117,3 @@ inline Complex spqr_conj (Complex x)

-inline double spqr_abs (double x, cholmod_common *cc)       // cc is unused
+inline double spqr_abs (double x, cholmod_common *)
 {
@@ -1122,3 +1122,3 @@ inline double spqr_abs (double x, cholmod_common *cc)       // cc is unused

-inline double spqr_abs (Complex x, cholmod_common *cc)
+inline double spqr_abs (Complex x, cholmod_common *)
 {
@@ -1132,3 +1132,3 @@ inline double spqr_abs (Complex x, cholmod_common *cc)

-inline double spqr_divide (double a, double b, cholmod_common *cc)  // cc unused
+inline double spqr_divide (double a, double b, cholmod_common *)
 {
@@ -1137,3 +1137,3 @@ inline double spqr_divide (double a, double b, cholmod_common *cc)  // cc unused

-inline Complex spqr_divide (Complex a, Complex b, cholmod_common *cc)
+inline Complex spqr_divide (Complex a, Complex b, cholmod_common *)
 {
diff --git a/SPQR/Source/SuiteSparseQR.cpp b/SPQR/Source/SuiteSparseQR.cpp
index a11317b02..1bde3bb0b 100644
--- a/SPQR/Source/SuiteSparseQR.cpp
+++ b/SPQR/Source/SuiteSparseQR.cpp
@@ -59,3 +59,4 @@
         spqr_free <Int> (n+bncols, sizeof (Int), E, cc) ; \
-        spqr_free <Int> (m, sizeof (Int), HP1inv, cc) ;
+        spqr_free <Int> (m, sizeof (Int), HP1inv, cc) ; \
+        (void)0

Apparently, you don't care about having a clean compilation process, which I'm not sure I understand why. The number of warnings (more than 50 for some files) makes it extremely difficult to follow along.

[...]
klu.c:769:47: warning: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Wextra-semi-stmt]
                DIV (X [4*k + 3], x [3], ukk) ;
                                              ^
76 warnings generated.
[...]
umfpack_report_symbolic.c:248:25: warning: empty expression statement has no effect; remove unnecessary ';' to silence this warning [-Wextra-semi-stmt]
    PRINTF (("OK\n\n")) ;
                        ^
54 warnings generated.
[...]

[mmuetzel]

There are no warnings with default compiler flags using GCC, Clang, or MSVC. It's unusual to impose a set of compiler flags from one project onto another... 🤔

[DrTimothyAldenDavis]

Yes I do care about silencing warnings but I don't get any of those warnings. I don't care about silencing pedantic warnings, except sometimes I add

pragma GCC warning disable ...

statements to the files themselves so they are permanently silenced regardless of compiler flags.

I don't want do-while(0) in my macros. I find them hard to understand.

[prj-]

There are no warnings with default compiler flags using GCC, Clang, or MSVC.

It is precisely because you use the default compiler flags that the initial major issue of this MR was not noticed.

I don't want do-while(0) in my macros. I find them hard to understand.

OK, what about unused parameters that don't have to be named in C++, but that are anyway named in spqr.hpp? Isn't that hard to understand?

[DrTimothyAldenDavis]

It turns out that those two functions with unused parameters (spqr_divide and spqr_abs) are not user-visible. So I can just remove the cc argument entirely. I used to use it but I don't anymore. That's a more robust way of silencing the warnings for those 2 functions.

[DrTimothyAldenDavis]

Which compiler (and version) are you using? gcc has -Wextra-semi but not -Wextra-semi-stmt .

[prj-]

The default clang for GitHub Actions with ubuntu-latest, which is at version 15.0.7, but I also get this on my own machines with clang latest snapshot (version 18).

[DrTimothyAldenDavis]

Thanks. I've resolved these warnings by introducing a -Wno-extra-semi-stmt flag when using clang. I realize this might not be seen as ideal, but I use many extra empty semi-colons in my code as a matter of style. It's a signal to future myself that I intentionally chose to do nothing at a certain point in the code. "Fixing" that might make the code pass the clang test but it would break my train of thought, which is vitally important when trying to revise the code in the future. So I don't have any alternative except -Wno-extra-semi-stmt. At least that will silence the compiler warnings you're seeing.

[prj-]

Just out ouf curiosity, how do you sanitize your code such that you are not forgetting an extra empty semi-colons? The do { } while (0) is horrendous, but at least, if you forget a semi-colon, PRINTF (("OK\n\n")) won't compile. Without this, how can you be certain that all of your macros have the proper extra (unneeded) semi-colons?

Edit: oh, I forgot the obvious, but thank you for adding the necessary #pragma clang diagnostic ignored (which I'm guessing is how you disabled the warnings).

[DrTimothyAldenDavis]

I don't require the extra semicolon inside a macro. I just use it intentionally sometimes, like this:

https://github.com/DrTimothyAldenDavis/SuiteSparse/blob/86d4718a0b00570fd07b52cbc97d7ba724f95ba1/SuiteSparse_config/SuiteSparse_config.c#L273-L289

elsewhere, I might use an empty semicolon on purpose, but not document it so carefully.