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.

Sign up for GitHub

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

Jump to bottom

Change type of offset arg in H5Pset_external to HDoff_t #3505

Merged
merged 4 commits into from
Jun 4, 2024
Merged

Change type of offset arg in H5Pset_external to HDoff_t #3505

merged 4 commits into from
Jun 4, 2024

Conversation

phil-opp
Copy link
Contributor

@phil-opp phil-opp commented Sep 5, 2023 •
edited
Loading

Describe your changes

The off_t type is only 32-bit on Windows, which makes it impossible to link to higher offsets in large files.

The H5O_efl_entry_t struct defines its offset field already as HDoff_t, so no additional conversion is needed.,

Issue ticket number (GitHub or JIRA)

#3506

Checklist before requesting a review

  • My code conforms to the guidelines in CONTRIBUTING.md
  • I made an entry in release_docs/RELEASE.txt (bug fixes, new features)
  • I added a test (bug fixes, new features)

@phil-opp phil-opp force-pushed the fix-H5Pset_external-offset branch 3 times, most recently from f2693dc to 2d6c308 Compare September 5, 2023 13:07
@phil-opp
Change type of offset arg in H5Pset_external to HDoff_t
8e4e9e5 
The `off_t` type is only 32-bit on Windows, which makes it impossible to link to higher offsets in large files.

The `H5O_efl_entry_t` struct defines its `offset` field already as `HDoff_t`, so no additional conversion is needed.
@phil-opp phil-opp force-pushed the fix-H5Pset_external-offset branch from 2d6c308 to 8e4e9e5 Compare September 5, 2023 13:14
@mkitti
Copy link
Contributor

mkitti commented Sep 6, 2023

I think we might need to version the function in order to change the API like this.

@derobins derobins added Merge - Develop Only This cannot be merged to previous versions of HDF5 (file format or breaking API changes) 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 Improvements that don't add a new feature or functionality WIP Work in progress (please also convert PRs to draft PRs) labels Sep 11, 2023
@derobins derobins marked this pull request as ready for review September 14, 2023 15:22
@derobins derobins removed the WIP Work in progress (please also convert PRs to draft PRs) label Sep 14, 2023
@derobins derobins marked this pull request as draft September 14, 2023 16:31
@derobins
Copy link
Member

Give us a little more time with this. We're thinking about ditching off_t entirely and moving to an unsigned integer.

@phil-opp
Copy link
Contributor Author

Sure, sounds good!

@derobins
Copy link
Member

Update: I'm starting to go through the library to make sure HDoff_t is used throughout. I think there's a few other internal changes we should also make. We have a lot of projects in motion right now, but I'll try to get the underlying work done in October so we can make this change.

@derobins
Copy link
Member

Update: The internal changes should be complete now. We're discussing how versioning is going to work for this.

@derobins derobins marked this pull request as ready for review March 28, 2024 14:52
derobins and others added 2 commits March 28, 2024 11:00
@derobins
Clean up HDoff_t PR
65650a3 
* Move HDoff_t to H5public.h
* Use typedefs instead of #define
* Remove duplicated definitions of HDoff_t
* Clean up h5_stat* definitions
* Add Doxygen for HDoff_t
@derobins
Merge branch 'develop' into fix-H5Pset_external-offset
bb6bd04
@derobins
Copy link
Member

This can probably go in at this point, but we still use off_t in places like the tools and tests. Those will have to get fixed in the near future.

@derobins
Copy link
Member

FWIW, this API call will NOT be versioned. If we didn't version API calls that use hid_t when we updated that from int to int64, we don't need to version this one.

jhendersonHDF
jhendersonHDF approved these changes Mar 28, 2024
View reviewed changes
Copy link
Collaborator

@jhendersonHDF jhendersonHDF left a comment

Choose a reason for hiding this comment

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

For going into develop this looks good. We'll likely want to think about how to make it so we don't accidentally sync this change back to the 1.14 branch.

@derobins derobins mentioned this pull request Mar 31, 2024
Merged
@derobins derobins merged commit b698b48 into HDFGroup:develop Jun 4, 2024
56 checks passed
@phil-opp phil-opp deleted the fix-H5Pset_external-offset branch June 5, 2024 11:18
byrnHDF pushed a commit to byrnHDF/hdf5 that referenced this pull request Jun 26, 2024
@phil-opp @byrnHDF
Change type of offset arg in H5Pset_external to HDoff_t (HDFGro…
dec06ce 
…up#3505)

The `off_t` type is only 32-bit on Windows, which makes it impossible to link to higher offsets in large files.

The `H5O_efl_entry_t` struct defines its `offset` field already as `HDoff_t`, so no additional conversion is needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhendersonHDF jhendersonHDF jhendersonHDF approved these changes

@lrknox lrknox lrknox approved these changes

@derobins derobins derobins approved these changes

@byrnHDF byrnHDF Awaiting requested review from byrnHDF byrnHDF is a code owner

@fortnern fortnern Awaiting requested review from fortnern fortnern is a code owner

@qkoziol qkoziol Awaiting requested review from qkoziol qkoziol is a code owner

@vchoi-hdfgroup vchoi-hdfgroup Awaiting requested review from vchoi-hdfgroup vchoi-hdfgroup is a code owner

@bmribler bmribler Awaiting requested review from bmribler bmribler is a code owner

@glennsong09 glennsong09 Awaiting requested review from glennsong09 glennsong09 is a code owner

@mattjala mattjala Awaiting requested review from mattjala mattjala is a code owner

@brtnfld brtnfld Awaiting requested review from brtnfld brtnfld is a code owner

Assignees

@derobins derobins

Labels
Component - C Library Core C library issues (usually in the src directory) Merge - Develop Only This cannot be merged to previous versions of HDF5 (file format or breaking API changes) Priority - 2. Medium ⏹ It would be nice to have this in the next release Type - Improvement Improvements that don't add a new feature or functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@phil-opp @mkitti @derobins @lrknox @jhendersonHDF