Refactor warning macros #5432

derobins · 2025-04-09T21:30:20Z

Replaces the scheme in H5warnings.h with a more compiler-agnostic one that relocates ifdef complexity to the warnings header file and uses better naming.

* Add cast alignment warnings for C++ tests * Fix the nonstandard suffix warnings * Update .clang-format * Remove the C99 extensions warning

Java is broken in CMake, so I can't debug this

…nto warning_refactor

Until whatever CMake + Java is doing is fixed

derobins · 2025-04-10T01:48:50Z

This would be ready to review if Java worked on Ubuntu, which it does not. Without a working Java setup, I can't debug the Java JNI header issue, where it appears to have a problem seeing H5warnings.h via H5private.h.

I'd also like to extend this to MSVC warnings, but I can't get HDF5 to compile with Visual Studio due to complex number errors. It's unclear why HDF5 builds in CI but not locally with Visual Studio.

jhendersonHDF · 2025-04-10T15:28:25Z

This would be ready to review if Java worked on Ubuntu, which it does not. Without a working Java setup, I can't debug the Java JNI header issue, where it appears to have a problem seeing H5warnings.h via H5private.h.

@byrnHDF or I might be able to take a look at java. What issue are you having with Java on Ubuntu?

I'd also like to extend this to MSVC warnings, but I can't get HDF5 to compile with Visual Studio due to complex number errors. It's unclear why HDF5 builds in CI but not locally with Visual Studio.

Could you create an issue with the problems you're seeing? I might be able to take a look if I can get Visual Studio working somewhere.

derobins · 2025-05-04T06:29:23Z

tools/src/h5repack/h5repack_filters.c

@@ -219,7 +217,6 @@ aux_assign_obj(const char  *name,        /* object name from traverse list */
        }
    }

-    *obj = tmp;


Not a good idea to return a stack object through a pointer...

This isn't returning a stack object, this is using struct assignment from tmp to the struct pointed to by obj, so it's perfectly fine. Plus it's a lot cleaner than the new solution if errors were to occur because all the fields were previously assigned all at once at the end here only on success, whereas the new solution could half-initialize the pointed to structure.

Yeah, I tend to agree

derobins · 2025-05-04T22:36:37Z

This is ready for review now.

FWIW, if we can fix a few stragglers in C++, Fortran, clang, and MSVC, we can start building with -Werror always and get rid of the special jobs.

qkoziol · 2025-05-05T19:46:09Z

testpar/t_subfiling_vfd.c

Using snprintf eliminates the warning?

Seconding this; is the warning just no longer issued?

No longer complains (if it did, the -Werror checks would fail)

I'm not sure that we're currently building parallel with -Werror or at least that it works correctly. Some of the parallel builds use the HDF5_ENABLE_WARNINGS_AS_ERRORS option, but I can see this warning in https://github.com/HDFGroup/hdf5/actions/runs/14818490824/job/41602216637, for example.

I think the option may simply not be covering all the cases we're interested in turning into an error from a warning. Looks like this particular warning may need to be suppressed again. We'll check into the issues with the option outside of this PR.

jhendersonHDF · 2025-05-05T20:00:15Z

src/H5warnings.h

-#define H5_DIAG_OFF(x) H5_DIAG_PRAGMA(push) H5_DIAG_PRAGMA(ignored H5_DIAG_JOINSTR("-W", x))
-#define H5_DIAG_ON(x)  H5_DIAG_PRAGMA(pop)
+#if defined(__clang__) || defined(__GNUC__)
+#define H5_WARN_CAST_ALIGNMENT_OFF H5_WARN_OFF("cast-align")


Out of all the warning types, cast-align is one that I think we should never allow suppressing warnings for. While it's fine on some platforms it's certainly not on others and will likely lead to subtle issues.

qkoziol

Looks good to me, modulo the possible revert on the pack option (but I don't have a strong opinion either way on that)

jhendersonHDF · 2025-05-05T20:42:14Z

c++/test/tcompound.cpp

@@ -98,7 +98,9 @@ test_compound_2()
        bkg  = static_cast<unsigned char *>(malloc(nelmts * sizeof(dst_typ_t)));
        orig = static_cast<unsigned char *>(malloc(nelmts * sizeof(src_typ_t)));
        for (i = 0; i < nelmts; i++) {
-            s_ptr       = (reinterpret_cast<src_typ_t *>(orig)) + i;
+            H5_WARN_CAST_ALIGNMENT_OFF
+            s_ptr = (reinterpret_cast<src_typ_t *>(orig)) + i;


If these few places are the only locations where cast-align is currently suppressed, I'd much rather these be fixed (maybe in a separate PR) than continuing to be suppressed. The solution is very simple so no real need to keep suppressing cast-align.

jhendersonHDF · 2025-05-05T20:42:59Z

Great changes that I'd like to see merged to fix the new annoying warnings from C23-related issues; just a couple minor things.

jhendersonHDF

I'll look into the remaining issue with the new warning later on; I'd much rather have the warning suppression reworking in.

derobins and others added 2 commits April 8, 2025 22:20

Refactor warning macros

13d651b

Replaces the scheme in H5warnings.h with a more compiler-agnostic one that relocates ifdef complexity to the warnings header file and uses better naming.

Committing clang-format changes

476f0fc

derobins added Priority - 2. Medium It would be nice to have this in the next release Component - C Library Core C library issues (usually in the src directory) Type - Improvement labels Apr 9, 2025

derobins and others added 8 commits April 9, 2025 15:04

Fix issues in H5warnings.h

0c6f2a0

Fix bugs and issues

5faebeb

* Add cast alignment warnings for C++ tests * Fix the nonstandard suffix warnings * Update .clang-format * Remove the C99 extensions warning

Disable Java warning suppression

f06036c

Java is broken in CMake, so I can't debug this

Committing clang-format changes

7eb1cae

Turn java off in -Werror builds

d968821

Merge remote-tracking branch 'refs/remotes/origin/warning_refactor' i…

6b840c2

…nto warning_refactor

Turn off -Werror for the test-cmake-minimum build

596ee2c

Turn Java off in test-cmake-minimum

f4c5adb

Until whatever CMake + Java is doing is fixed

brtnfld removed the Type - Improvement label Apr 25, 2025

brtnfld assigned fortnern May 2, 2025

derobins added 2 commits May 3, 2025 19:44

Merge remote-tracking branch 'canonical/develop' into warning_refactor

8eaa6c8

Fix Java typo

b2ff68f

derobins marked this pull request as ready for review May 4, 2025 05:09

derobins requested review from jhendersonHDF, byrnHDF, lrknox, fortnern, qkoziol, vchoi-hdfgroup, bmribler, glennsong09, mattjala and brtnfld as code owners May 4, 2025 05:09

derobins added 4 commits May 3, 2025 23:16

Fix stack size warnings in h5repack

3694d04

Turn Java back on in -Werror builds

3d6ca42

Turn Java back on in test-cmake-minimum

e402132

Fix sign conversion macros in JNI code

0976706

derobins commented May 4, 2025

View reviewed changes

qkoziol reviewed May 5, 2025

View reviewed changes

jhendersonHDF reviewed May 5, 2025

View reviewed changes

qkoziol approved these changes May 5, 2025

View reviewed changes

jhendersonHDF reviewed May 5, 2025

View reviewed changes

jhendersonHDF approved these changes May 23, 2025

View reviewed changes

jhendersonHDF merged commit d9f5f50 into HDFGroup:develop May 23, 2025
60 checks passed

Uh oh!

Refactor warning macros #5432

Refactor warning macros #5432

Uh oh!

Conversation

derobins commented Apr 9, 2025

Uh oh!

derobins commented Apr 10, 2025

Uh oh!

jhendersonHDF commented Apr 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhendersonHDF May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derobins commented May 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qkoziol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhendersonHDF commented May 5, 2025

Uh oh!

jhendersonHDF left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jhendersonHDF May 5, 2025 •

edited

Loading