Skip to content

Rework the warning suppression macros #5424

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
derobins opened this issue Apr 8, 2025 · 5 comments · May be fixed by #5432
Open

Rework the warning suppression macros #5424

derobins opened this issue Apr 8, 2025 · 5 comments · May be fixed by #5432
Assignees
Labels
Component - C Library Core C library issues (usually in the src directory) Priority - 2. Medium It would be nice to have this in the next release

Comments

@derobins
Copy link
Member

derobins commented Apr 8, 2025

The macros in H5warnings.h have several flaws:

  • The compiler name(s) appear in the macros, which means that we sometimes either have multiple macro lines around problematic code or we wind up with "combo" macros like the gcc-clang macros that will be awkward to extend to new compilers.
  • The macros are simple replacements for individual pragma lines, so any ifdef complications have to be handled in the library code, not H5warnings.h. These ifdefs and the comments that explain them will have to be duplicated throughout the library code.
  • The name of the suppressed warning may not be very informative. For example, passing MPI_STATUSES_IGNORE as a parameter to MPI calls can raise spurious stringop-overflow warnings w/ gcc. It'd be nice to have a warning suppression that clearly indicated the nature of the (non-)problem.

We should replace the existing scheme with one that uses a wider set of macros that hide ifdef complexity and can be more easily extended to other compilers (e.g., MSVC).

Example:

/* Disable warnings concerning non-standard extensions, like F16 */
/* clang */
#if defined(__clang__)
#define H5_WARN_NONSTD_SUFFIX_OFF H5_WARN_OFF("pedantic")
#define H5_WARN_NONSTD_SUFFIX_ON  H5_WARN_ON("pedantic")
/* gcc 14+ */
#elif defined(__GNUC__) && __GNUC__ >= 14
#define H5_WARN_NONSTD_SUFFIX_OFF H5_WARN_OFF("c11-c23-compat")
#define H5_WARN_NONSTD_SUFFIX_ON  H5_WARN_ON("c11-c23-compat")
/* gcc 9-13 */
#elif defined(__GNUC__) && __GNUC__ >= 9
#define H5_WARN_NONSTD_SUFFIX_OFF H5_WARN_OFF("c11-c2x-compat")
#define H5_WARN_NONSTD_SUFFIX_ON  H5_WARN_ON("c11-c2x-compat")
#else
/* Everything else */
#define H5_WARN_NONSTD_SUFFIX_OFF
#define H5_WARN_NONSTD_SUFFIX_ON
#endif

@derobins derobins self-assigned this Apr 8, 2025
@derobins 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 8, 2025
@jhendersonHDF
Copy link
Collaborator

Agreed. The c11-c2x-compat / c11-c23-compat flag is an annoyance due to the changed and deprecated name between versions.

@qkoziol
Copy link
Collaborator

qkoziol commented Apr 8, 2025

Agree - something like this will be more scalable

@byrnHDF
Copy link
Contributor

byrnHDF commented Apr 8, 2025

Can't we use a general name and create that actual details in the compiler-specific files.

@derobins
Copy link
Member Author

derobins commented Apr 8, 2025

Can't we use a general name and create that actual details in the compiler-specific files.

I've thought about how to do that, but it would be awkward and probably awful to maintain. Different warnings will have different ifdef soup and the macro names are inside the ifdefs. I'd much rather maintain a dozen macros with a similar layout than a complicated, error-prone cut-n-paste macro scheme.

@derobins
Copy link
Member Author

derobins commented Apr 8, 2025

I have this work essentially completed. I'll try to get a PR in this week.

@derobins derobins linked a pull request Apr 9, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - C Library Core C library issues (usually in the src directory) Priority - 2. Medium It would be nice to have this in the next release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants