Split H5Tconv.c into modules by type #4393
Conversation
jhendersonHDF
added
Merge - To 1.14
Priority - 3. Low 🔽
jhendersonHDF
requested review from
lrknox,
derobins,
byrnHDF,
fortnern,
qkoziol,
vchoi-hdfgroup,
bmribler,
glennsong09,
mattjala and
brtnfld
as code owners
|
Happy to discuss other potential ways of organizing this, but this split makes it easier to find a particular conversion function, prevents H5Tconv.c from getting out of hand due to the combinations of conversions between types and also speeds up parallelized building of the library a little bit since it doesn't get hung up on H5Tconv.c. It also moves several functions, typedefs, etc. related to datatype conversions into the new H5Tconv.h header. |
src/H5T.c
Outdated
| #include "H5Tconv_llong.h" | ||
| #include "H5Tconv_float.h" | ||
| #include "H5Tconv_double.h" | ||
| #include "H5Tconv_ldouble.h" |
There was a problem hiding this comment.
Since all the conversion functions are currently only needed during H5T_init, I moved the prototypes into separate headers rather than H5Tconv.h so we only include them where we use them.
src/H5Tconv.c
Outdated
There was a problem hiding this comment.
I'd suggest moving the struct, string, bitfield, etc. conversions into their own files also.
There was a problem hiding this comment.
Any potential name ideas? H5Tconv_soft, H5Tconv_general, ...?
There was a problem hiding this comment.
No strong feelings, but I would suggest putting each remaining datatype class into it's own H5Tconv_.c file and just leave the utility routines and order "conversion" in H5Tconv.c
There was a problem hiding this comment.
I mostly left these in H5Tconv.c because I figured adding a new source/header combination for a single conversion function would probably be overkill, though I suppose some of them have helper functions for managing their cached private data and some have optimized versions of the functions. Really, I think it's an argument toward finally separating source files into directories by package, but that's for another day and another PR.
There was a problem hiding this comment.
No strong feelings, but I would suggest putting each remaining datatype class into it's own H5Tconv_.c file and just leave the utility routines and order "conversion" in H5Tconv.c
Done. Reorganized so that this is strictly organized by datatype class. It moves the integer and float conversions back into single, larger files, but it's still easier to find what you're looking for compared to before. It also doesn't appear to have much of a negative effect on parallelized compile time either since the source files for the other datatype classes can be processed at the same time. Decided to do this since it then makes sense where to place the software conversion functions like "i_i" and "i_f".
There was a problem hiding this comment.
Makes good sense to me, and agree that we're definitely still ahead even if the integer and float files are a little large.
jhendersonHDF
force-pushed
the
H5Tconv_split
branch
2 times, most recently
from
6ccea4d to
83c9019
Compare
| ${HDF5_SRC_DIR}/H5Tconv_reference.c | ||
| ${HDF5_SRC_DIR}/H5Tconv_enum.c | ||
| ${HDF5_SRC_DIR}/H5Tconv_vlen.c | ||
| ${HDF5_SRC_DIR}/H5Tconv_array.c |
There was a problem hiding this comment.
The header files should be added to the list of private header files as well.
|
Approved, but should add the private headers to the CMake lists so Visual Studio, etc. will show them in the IDE. |
* Split H5Tconv.c into modules by type * Add new H5Tconv headers to list of private headers
* Split H5Tconv.c into modules by type (#4393) * Split H5Tconv.c into modules by type * Add new H5Tconv headers to list of private headers * Fix broken links in VOL API table (#4438) * Don't print thread ID when the library isn't multithreaded. (#4428) Corresponding changes to make error output for regression tests agnostic to thread setting. Signed-off-by: Quincey Koziol <quincey@koziol.cc> * Start refactoring H5E code to avoid using IDs internally (#4427) * Add support for builtin_expect compiler hint (#4425) * Add support for __builtin_expect extension And H5_LIKELY / H5_UNLIKELY macros to wrap it Signed-off-by: Quincey Koziol <quincey@koziol.cc> * Committing clang-format changes --------- Signed-off-by: Quincey Koziol <quincey@koziol.cc> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * sanitizer flags need set before compiler flags (#4444) * Add navigate chapters and use release_docs in Learn Basics (#4441) * Fix for github issue #3790: infinite loop closing library (#4445) * Fix for github issue #3790: infinite loop closing library Cause of the problem: When h5dump tries to open the user provided test file, the metadata cache will call the "get_final_load_size" callback to find out the actual size of the the root object header. The callback function will call H5O__prefix_deserialize() to allocate space for the object header data structure (via H5FL_CALLOC) and to deserialize the object header prefix in order to find the actual size of the object header. The metadata cache will then check whether the actual size obtained will exceed the file's EOA. Since the actual size obtained from the test file exceeds the EOA, the metadata cache throws an error and return. However, the oh structure that was allocated in H5O__prefix_deserialize() was not freed and hence causing the problem described in this issue. Fix: 1) Deallocate the oh structure after obtaining and saving the needed information in udata which will be used later on in the "verify_chksum" callback. 2) Deserialize the object header prefix in the "object header's "deserialize" callback regardless. The original coding intends to keep the deserialized prefix so that the object header's "deserialize" callback does not need to deserialize the prefix again if the object header is coming through the "get_final_load_size" callback. * H5R Fortran wrappers and misc. H5R API/DOC updates (#4446) - Add Fortran H5R APIs: h5rcreate_attr_f, h5rcreate_object_f, h5rcreate_region_f, h5ropen_attr_f, h5ropen_object_f, h5ropen_region_f, h5rget_file_name_f, h5rget_attr_name_f, h5rget_obj_name_f, h5rcopy_f, h5requal_f, h5rdestroy_f, h5rget_type_f - Fixed function H5Requal actually to compare the reference pointers Fixed an issue with H5Requal always returning true because the function was only comparing the ref2_ptr to itself. * Fix heap-buffer-overflow in H5Fio.c (#4450) The buffer size for checksum was smaller than H5_SIZEOF_CHKSUM, causing an overflow while calculating the offset to the checksum in the buffer. A check was added so H5F_get_checksums would fail appropriately in all of its occurrences. Fix gh-4434 * Fix grammar in VOL guide (#4452) * Fix bug in MPI-IO VFD (#4456) Corrects incorrect usage of the vector_was_sorted parameter in H5FD__mpio_vector_build_types() * Bump the github-actions group with 3 updates (#4455) Bumps the github-actions group with 3 updates: [actions/download-artifact](https://github.com/actions/download-artifact), [peaceiris/actions-gh-pages](https://github.com/peaceiris/actions-gh-pages) and [github/codeql-action](https://github.com/github/codeql-action). Updates `actions/download-artifact` from 4.1.4 to 4.1.7 - [Release notes](https://github.com/actions/download-artifact/releases) - [Commits](actions/download-artifact@c850b93...65a9edc) Updates `peaceiris/actions-gh-pages` from 3.9.3 to 4.0.0 - [Release notes](https://github.com/peaceiris/actions-gh-pages/releases) - [Changelog](https://github.com/peaceiris/actions-gh-pages/blob/main/CHANGELOG.md) - [Commits](peaceiris/actions-gh-pages@373f7f2...4f9cc66) Updates `github/codeql-action` from 3.24.9 to 3.25.3 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@1b1aada...d39d31e) --- updated-dependencies: - dependency-name: actions/download-artifact dependency-type: direct:production update-type: version-update:semver-patch dependency-group: github-actions - dependency-name: peaceiris/actions-gh-pages dependency-type: direct:production update-type: version-update:semver-major dependency-group: github-actions - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-minor dependency-group: github-actions ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Fixed failures with xl compilers. (#4458) * type cast constant * fixed return types * Convert ERR test to use grep (#4451) * Convert ERR test to use grep * Eliminate use of .err files in CMake * Show error output if grep fails * Turn off cuda in NVHPC CI * Removed "function/code stack" debugging configure option (#4454) Easily replaced w/third-party tools, e.g. libbacktrace (https://github.com/ianlancetaylor/libbacktrace) * Clean up memory leaks in t_vfd (#4457) * Fixes and cleanup for ph5diff (#4460) * Fixes and cleanup for ph5diff Fixes concurrency issues in ph5diff that can cause interleaved output Fixes an issue where output can sometimes be dropped if it ended up in ph5diff's output overflow file Fixes an issue where MPI_Init is called after HDF5 has been initialized, preventing the library from setting up an MPI attribute to perform cleanup on MPI_Finalize Fixes an issue in config/cmake/runTest.cmake where the CMake logic would try to access an invalid list index if the number of lines in a test's output and reference files don't match * Add release note * Remove use of err files in autotools test scripts (#4461) * Fix typo in H5Rget_obj_type (#4463) Issue GH-1723 * Use ADD_H5_ERR_TEST to not compare output (#4464)
* Split H5Tconv.c into modules by type * Add new H5Tconv headers to list of private headers
No description provided.