Skip to content
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

Silence most warnings in libhdf5 #2874

Merged
merged 3 commits into from
Mar 20, 2024
Merged

Conversation

ZedThree
Copy link
Contributor

@ZedThree ZedThree commented Mar 1, 2024

The remaining warnings basically fall into two categories:

  • those from NC_ATT_INFO.len being int -- changing this to size_t would silence a lot of warnings in one go
  • those from HDF5 filters -- the filter type in HDF5 itself is H5Zfilter_t == int, whereas netcdf mostly uses unsigned int; and then the flags/parameters are exactly the other way round.

The filter types can't be completely fixed because they're in the netcdf public API, but we could fix the internals and just convert on the boundary. This is would change a bunch of internal API though, so I haven't done that here.

@DennisHeimbigner
Copy link
Collaborator

The change to NC_ATT_INFO is probably ok since it is never visible on disk or to users.
Changing the filter type is more problematic. It is visible to both the users our libnetcdf
and libnetcdf's use of libhdf5, so the conversion would have occur in both situations.
I think this also affects the filter wrappers in the plugins directory.
Bottom line, this change may not be worth it at the moment.

@edwardhartnett
Copy link
Contributor

I agree about the NC_ATT_INFO len.

WRT to types for filter functions, the goal is to hide all HDF5 types behind standard C types. That is, I deliberately avoided using any HDF5 types in the netCDF API. I'm not sure if the filter code follows this rule.

I know of several advanced users using filters to great effect, including some ESA satellites which are using JPEG compression in netCDF data (IIRC in the recently-launched MTG-I1). So this is critical functionality that is already built into important operational systems.

Fixes about 20 warnings
@ZedThree
Copy link
Contributor Author

ZedThree commented Mar 4, 2024

Ok, I changed NC_ATT_INFO.len -- it fixed 20 warnings and raised no new ones.

I might make a separate PR for the internal type of the filter ID so you can see the specific changes

@WardF WardF merged commit fa6df05 into Unidata:main Mar 20, 2024
103 checks passed
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.

4 participants