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

Rewrite H5T__path_find_real for clarity #4225

Merged
merged 3 commits into from
Mar 27, 2024
Merged

Rewrite H5T__path_find_real for clarity #4225

merged 3 commits into from
Mar 27, 2024

Conversation

jhendersonHDF
Copy link
Collaborator

No description provided.

@jhendersonHDF jhendersonHDF added Merge - To 1.14 Priority - 3. Low 🔽 Code cleanup, small feature change requests, etc. Component - C Library Core C library issues (usually in the src directory) Type - Improvement Improvements that don't add a new feature or functionality labels Mar 24, 2024
@jhendersonHDF
Copy link
Collaborator Author

I personally find H5T__path_find_real a bit hard to parse through at times, so I rewrote it a bit in a way that I believe is a lot easier to work through.

jhendersonHDF
jhendersonHDF commented Mar 24, 2024
View reviewed changes
src/H5T.c
*-------------------------------------------------------------------------
*/
static void *
H5T__bsearch_path_table(const H5T_t *src, const H5T_t *dst, int *last_cmp, int *idx)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved the binary search logic into this helper function. I wanted to just implement this using bsearch, but its interface is a bit awkward to use for the things we're interested in knowing about like the last comparison value and the index into the table where we landed.

jhendersonHDF
jhendersonHDF commented Mar 24, 2024
View reviewed changes
src/H5T.c
/*
* Make sure the first entry in the table is the no-op conversion path.
*/
if (0 == H5T_g.npaths) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved the allocation and initialization of the path table into a helper function

jhendersonHDF
jhendersonHDF commented Mar 24, 2024
View reviewed changes
src/H5T.c
path = matched_path;

/* Initialize the path if it's a new path */
if (new_path && H5T__path_find_init_new_path(path, src, dst, conv, &tmp_ctx) < 0)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved the two large sections that initialize a new path with either the specified hard conversion function or an existing soft function into the new H5T__path_find_init_new_path helper function, making the purpose of those sections clearer and localizing the temporary datatypes and IDs for those types used when initializing the conversion function

qkoziol
qkoziol reviewed Mar 25, 2024
View reviewed changes
src/H5T.c Outdated Show resolved Hide resolved
qkoziol
qkoziol reviewed Mar 25, 2024
View reviewed changes
src/H5T.c Outdated
assert(matched_path == H5T_g.path[path_idx]);
H5T__print_stats(matched_path, &nprint);

/* Free the old conversion path table entry */
Copy link
Contributor

Choose a reason for hiding this comment

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

This block of code, to free a conversion path entry, is replicated at least 4 times in this file (search for H5T_CONV_FREE) and would be good to move to a helper routine also.

qkoziol
qkoziol approved these changes Mar 25, 2024
View reviewed changes
Copy link
Contributor

@qkoziol qkoziol left a comment

Choose a reason for hiding this comment

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

Solid improvement, even without the suggestions I made.

@jhendersonHDF jhendersonHDF force-pushed the H5T_path_find_real_clarity branch from f46769b to fefe9e9 Compare March 27, 2024 03:09
@lrknox lrknox merged commit 37df217 into HDFGroup:develop Mar 27, 2024
52 checks passed
@jhendersonHDF jhendersonHDF deleted the H5T_path_find_real_clarity branch March 27, 2024 14:48
lrknox pushed a commit to lrknox/hdf5 that referenced this pull request Mar 29, 2024
@jhendersonHDF @lrknox
Rewrite H5T__path_find_real for clarity (HDFGroup#4225)
0a94294 
* Move conversion path free logic to helper function
lrknox added a commit that referenced this pull request Mar 29, 2024
@lrknox
Sync develop changes March 25 - March 28 to hdf5_1_14 (#4284)
dded937 
* Take user block into account when returning chunk addresses (#4236)

Both H5Dchunk_iter() and H5Dget_chunk_info(_by_coord)() did not take
the size of the user block into account when reporting addresses. Since
the #1 use of these functions is to root around in the file for the raw
data, this is kind of a problem.

Fixes GitHub issue #3003

* Fix a minor warning in h5test.c (#4242)

* Turn on -Werror for Java in GitHub -Werror workflows (#4243)

* Update Windows CI to not install ninja (#4230)

* Rework Fortran macros to use the proper code. (#4240)

* Correct reference copy for 16 API (#4244)

* Determine MPI LOGICAL during build, used in tests. (#4246)

* Skip userblock test in chunk_info.c for multi-file VFDs (#4249)

* Match generators with real cmake -G output on Windows (#4252)

* Add Julia GitHub Actions. (#4123)

* Re-revert to using autoreconf in autogen.sh (#4253)

We previously tried removing the per-tool invocation of the Autotools
and instead simply invoked autoreconf (PR #1906). This was reverted
when it turned out that the NAG Fortran compiler had trouble with an
undecorated -shared linker flag.

It turns out that this is due to a bug in libtool 2.4.2 and earlier.
Since this version of libtool is over a decade old, we're un-reverting
the change. We've added a release note for anyone who has to build
from source on elderly platforms.

Fixes #1343

* Rewrite H5T__path_find_real for clarity (#4225)

* Move conversion path free logic to helper function

* Add tgz extensions on names (#4255)

* Remove an error check regarding large cache objects (#4254)

* Remove an error check regarding large cache objects

In PR#4231 an assert() call was converted to a normal HDF5 error
check. It turns out that the original assert() was added by a
developer as a way of being alerted that large cache objects
existed instead of as a guard against incorrect behavior, making
it unnecessary in either debug or release builds.

The error check has been removed.

* Update RELEASE.txt

* File format security issues (#4234)

* Add job timeout to cygwin workflow (#4260)

* Replace user-define with user-defined (#4261)

* Improve the CMake clang -fsanitize=memory flags (#4267)

-fsanitize=memory is almost useless without
using -fsanitize-memory-track-origins=2 and we shoud probably add
-fno-optimize-sibling-calls as well.

* Add documentation (H5M) (#4259)

* Add documentation (H5P) (#4262)

* MPI type correction (#4268)

* corrected type for MPI_*_f2c APIs

* fixed return type of callback

* reset compilation flags of logical test program

* Clean up test/cmpd_dtransform.c (#4270)

* Clean up test/cmpd_dtransform.c

* Fix uninitialized memory warning from sanitizers
* FAIL_STACK_ERROR --> TEST_ERROR
* Emit output
* Delete test file when done

* Fix typo

* H5Fdelete() --> remove()

* Fix uninitialized memory issues in packet table (#4271)

* replace deprecated CMAKE_COMPILER_IS_GNU** (#4272)

* Prevent stack overflows in H5E__push_stack (#4264)

* Minor fixes after merge of file format security fixes (#4263)

* Update H5_IS_BUFFER_OVERFLOW to account for 'size' of 0

* Invert a few checks to avoid function call

* CHECK --> CHECK_PTR in tmisc.c (#4274)

* Add release note for CVE-2017-17507 (#4275)

* Update Cygwin installation guide (#4265)

* Addresses configuration fortran testing flags (#4276)

* turn warnings to errors in fortran configure test

* Intel fortran test fix

* Merge julia workflows into standard ci format (#4273)

* Fix range check in H5_addr_overlap (#4278)

When the H5_addr_overlap macro was updated to use H5_RANGE_OVERLAP,
it failed to take into account that H5_RANGE_OVERLAP expects the
range to be inclusive. This lead to an assertion failure in
H5MM_memcpy due to a memcpy operation on overlapping memory.
This has been fixed by subtracting 1 from the calculated high
bound values passed to H5_RANGE_OVERLAP

* Fix potential buffer read overflows in H5PB_read (#4279)

H5PB_read previously did not account for the fact that the size of the
read it's performing could overflow the page buffer pointer, depending
on the calculated offset for the read. This has been fixed by adjusting
the size of the read if it's determined that it would overflow the page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@byrnHDF byrnHDF byrnHDF approved these changes

@qkoziol qkoziol qkoziol approved these changes

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

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

@fortnern fortnern Awaiting requested review from fortnern fortnern 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
No one assigned
Labels
Component - C Library Core C library issues (usually in the src directory) Priority - 3. Low 🔽 Code cleanup, small feature change requests, etc. Type - Improvement Improvements that don't add a new feature or functionality
Projects
Downstream HDF5 Merges
Status: Needs Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jhendersonHDF @qkoziol @byrnHDF @lrknox