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

Switch HDoff_t to uint64_t in the public API #5082

Draft
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

derobins
Copy link
Member

@derobins derobins commented Nov 7, 2024

Moves HDoff_t out of the public API and replaces the HDoff_t
parameter type in H5Pset/get_external() with a uint64_t.

Also removes OFF_T from the Fortran public API.

@derobins derobins added Priority - 1. High 🔼 These are important issues that should be resolved in the next release Component - C Library Core C library issues (usually in the src directory) Component - Tools Command-line tools like h5dump, includes high-level tools Component - Testing Code in test or testpar directories, GitHub workflows Type - Improvement Improvements that don't add a new feature or functionality labels Nov 7, 2024
@derobins derobins requested a review from epourmal as a code owner November 8, 2024 07:43
@derobins derobins added Component - Java Java wrappers Component - Fortran Fortran wrappers Component - C++ C++ wrappers labels Nov 8, 2024
byrnHDF
byrnHDF previously approved these changes Nov 8, 2024
mattjala
mattjala previously approved these changes Nov 8, 2024
src/H5private.h Outdated Show resolved Hide resolved
brtnfld
brtnfld previously approved these changes Nov 8, 2024
@derobins derobins dismissed stale reviews from brtnfld, mattjala, and byrnHDF via e477946 November 8, 2024 15:45
@epourmal
Copy link

epourmal commented Nov 8, 2024

I am puzzled with this the switch from off_t to haddr_t. The parameter is used for the offsets in external files that ARE not HDF5 files. haddr_t corresponds to an offset in a logical HDF5 file. Are you sure, this is a correct approach?

@derobins
Copy link
Member Author

derobins commented Nov 8, 2024

I am puzzled with this the switch from off_t to haddr_t. The parameter is used for the offsets in external files that ARE not HDF5 files. haddr_t corresponds to an offset in a logical HDF5 file. Are you sure, this is a correct approach?

off_t isn't portable, so it really should be replaced with something that is (HDoff_t is what we use internally). I originally had it as HDoff_t, which I moved to the public API, then wanted to switch the name to something more in line with the other HDF5 primitive types (hoff_t, like hsize_t) and got pushback from Quincey about the semantics of HD* vs h*. We compromised on haddr_t, which is logically reasonable, a 64-bit type, and portable.

We're not using the offset parameter to seek to negative offsets, so the semantics of off_t aren't really necessary and haddr_t makes more semantic sense than an arbitrary integer.

I see your point, though. A haddr_t isn't really a physical offset if there's a user block, so the semantics are a bit different. I just really want to get rid of off_t. Non-standard-C types are annoying to support.

@epourmal
Copy link

epourmal commented Nov 8, 2024

I just really want to get rid of off_t. Non-standard-C types are annoying to support.

I don't think we can have a good solution here.

@epourmal
Copy link

epourmal commented Nov 8, 2024

Let's use uint64_t

Copy link
Member

@fortnern fortnern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to make sure not to merge this to 1.14

@derobins
Copy link
Member Author

derobins commented Nov 8, 2024

We'll need to make sure not to merge this to 1.14

That won't be a problem. We don't maintain the 1.14 branch anymore and a 1.14.6 release (if that happens) will basically be a patch release with a few bugfixes cherry-picked into a copy of the 1.14.5 release branch.

Copy link
Contributor

@brtnfld brtnfld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add to the Release.txt the Fortran changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add to the Fortran Section:

The offset parameter was changed for H5P(set|get)_external_f() from an INTEGER of KIND OFF_T to HADDR_T, and the OFF_T parameter is no longer a defined INTEGER KIND.

@derobins derobins marked this pull request as draft November 11, 2024 13:52
@derobins derobins changed the title Switch HDoff_t to haddr_t in the public API Switch HDoff_t to uint64_t in the public API Nov 19, 2024
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) Component - C++ C++ wrappers Component - Fortran Fortran wrappers Component - Java Java wrappers Component - Testing Code in test or testpar directories, GitHub workflows Component - Tools Command-line tools like h5dump, includes high-level tools Priority - 1. High 🔼 These are important issues that should be resolved in the next release Type - Improvement Improvements that don't add a new feature or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants