Skip to content

Switch HDoff_t to uint64_t in the public API #5082

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

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 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 Component - Wrappers C++, Java & Fortran wrappers labels Nov 8, 2024
byrnHDF
byrnHDF previously approved these changes Nov 8, 2024
mattjala
mattjala previously approved these changes Nov 8, 2024
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
Collaborator

@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
Collaborator

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
@fortnern fortnern self-assigned this Mar 11, 2025
@gheber gheber requested a review from Copilot June 11, 2025 22:28
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the deprecated HDoff_t from the public API and standardizes external‐file offsets on haddr_t (a 64-bit type), moving the old typedef into the private header. It also updates all core C functions, the stdio driver, Java JNI, Fortran bindings, C++ interfaces, examples, and documentation to use the new type.

  • Relocated HDoff_t to src/H5private.h and removed it from H5public.h.
  • Changed all H5Pset/get_external and related internal APIs to use haddr_t.
  • Updated language bindings (Java, Fortran, C++) and examples, plus revised release notes.

Reviewed Changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/H5public.h Removed HDoff_t from the public header
src/H5private.h Added private typedef of HDoff_t
src/H5Ppublic.h Changed external‐file offset parameters to haddr_t
src/H5Pdcpl.c Updated internal property list code to use haddr_t
src/H5Oprivate.h Switched internal EFL entry offset field to haddr_t
src/H5Oefl.c Adjusted decode logic to assign haddr_t offsets
src/H5FDstdio.c Refactored stdio driver to use my_off_t and haddr_t
src/H5Defl.c Updated external read/write seeks for new offset type
release_docs/RELEASE.txt Revised notes on external offset type change
java/src/jni/h5pDCPLImp.c Modified JNI calls to use haddr_t
fortran/test/tH5P_F03.F90 Updated tests to pass haddr_t offsets
fortran/test/tH5P.F90 Switched test variables from OFF_T to HADDR_T
fortran/src/H5match_types.c Removed matching of off_t in Fortran type mapping
fortran/src/H5f90proto.h Changed Fortran prototypes to use haddr_t_f
fortran/src/H5Pff.F90 Updated Fortran binding routines to HADDR_T
fortran/src/H5Pf.c Adjusted C wrappers to use haddr_t_f
c++/src/H5DcreatProp.h Replaced HDoff_t with haddr_t in C++ API signatures
c++/src/H5DcreatProp.cpp Updated C++ implementation to pass haddr_t
c++/src/C2Cppfunction_map.htm Corrected generated map entries to haddr_t
HDF5Examples/FORTRAN/H5D/h5ex_d_extern.F90 Example code switched to HADDR_T
Comments suppressed due to low confidence (2)

src/H5private.h:606

  • [nitpick] The comment refers to HDoff_t being typedef'd "above," but the public definition was removed; consider clarifying that HDoff_t is now defined in H5private.h to avoid confusion.
* off_t. Both of these are typedef'd to HDoff_t (above).

release_docs/RELEASE.txt:363

  • [nitpick] The release notes describe the change to haddr_t for external offsets but omit mention of removing OFF_T from the Fortran public API; consider adding a bullet about replacing OFF_T with HADDR_T in Fortran bindings.
-    - H5P(set|get)_external() now take a haddr_t offset parameter, which is

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 - Testing Code in test or testpar directories, GitHub workflows Component - Tools Command-line tools like h5dump, includes high-level tools Component - Wrappers C++, Java & Fortran wrappers Priority - 1. High These are important issues that should be resolved in the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants