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

Make h5dump spacing consistent when printing VLEN datatype (#3351) #3352

Merged
merged 1 commit into from
Aug 4, 2023
Merged

Make h5dump spacing consistent when printing VLEN datatype (#3351) #3352

merged 1 commit into from
Aug 4, 2023

Conversation

jhendersonHDF
Copy link
Collaborator

No description provided.

@jhendersonHDF jhendersonHDF added Merge Use this label when a PR is for a downstream merge Priority - 3. Low 🔽 Code cleanup, small feature change requests, etc. Component - Tools Command-line tools like h5dump, includes high-level tools Type - Task Actions that don't fit into any other type category Branch - 1.14 labels Aug 4, 2023
byrnHDF
byrnHDF reviewed Aug 4, 2023
View reviewed changes
tools/lib/h5tools_dump.c
@@ -2571,7 +2571,7 @@ h5tools_print_datatype(FILE *stream, h5tools_str_t *buffer, const h5tool_format_
if (H5Tclose(super) < 0)
H5TOOLS_ERROR((-1), "H5Tclose failed");

h5tools_str_append(buffer, "%s", h5tools_dump_header_format->vlenblockend);
h5tools_str_append(buffer, " %s", h5tools_dump_header_format->vlenblockend);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have been changed where vlenblockend is defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

See line 159 of same file.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, line 2567 is wrong and line 158 should be "H5T_VLEN { "

Copy link
Contributor

Choose a reason for hiding this comment

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

Another one: line 2239 should change line 152 to be: "H5T_STRING {"

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually that whole function, h5tools_print_datatype, needs to fixed.

byrnHDF
byrnHDF requested changes Aug 4, 2023
View reviewed changes
tools/lib/h5tools_dump.c
@@ -2571,7 +2571,7 @@ h5tools_print_datatype(FILE *stream, h5tools_str_t *buffer, const h5tool_format_
if (H5Tclose(super) < 0)
H5TOOLS_ERROR((-1), "H5Tclose failed");

h5tools_str_append(buffer, "%s", h5tools_dump_header_format->vlenblockend);
h5tools_str_append(buffer, " %s", h5tools_dump_header_format->vlenblockend);
Copy link
Contributor

Choose a reason for hiding this comment

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

See line 159 of same file.

@byrnHDF
Copy link
Contributor

byrnHDF commented Aug 4, 2023

Wow, looking at again, the h5tools_dump_header_t define should not have included types like string and vlen and enum

@byrnHDF
Copy link
Contributor

byrnHDF commented Aug 4, 2023

I will fix this in develop.

@byrnHDF
Copy link
Contributor

byrnHDF commented Aug 4, 2023

This change is okay with #3353 created to fix issues

@jhendersonHDF
Copy link
Collaborator Author

I think the main issue here was just that the printing code isn't always consistent about who is responsible for adding spacing. It should be caller or callee, but not both. If the code in h5tools_print_datatype adds a space beforehand, like in "H5T_ARRAY { ", then it should also be responsible for adding the end spacing, as in " }" like it does now. Otherwise, it shouldn't add either space and let the actual datatype printing code do that. Otherwise you get these problems, especially when things are recursive.

@derobins derobins merged commit 7aedee8 into HDFGroup:hdf5_1_14 Aug 4, 2023
@byrnHDF
Copy link
Contributor

byrnHDF commented Aug 4, 2023

I think that was the intention and the reason for my initial comments.
So there was two solutions, create PR #3353 and make everything use the formatting in the function
or add the missing types to dataformat struct and use the specific struct entry each with just a print "%s". Which would be correct. I went with the PR so as not to change the sorta-public API.

@jhendersonHDF
Copy link
Collaborator Author

I think that was the intention and the reason for my initial comments. So there was two solutions, create PR #3353 and make everything use the formatting in the function or add the missing types to dataformat struct and use the specific struct entry each with just a print "%s". Which would be correct. I went with the PR so as not to change the sorta-public API.

I think the struct types aren't meant to have any spacing in them. They're just for changing what the "begin block" and "end block" delimiters are and the formatting always needs to be down in either h5tools_print_datatype or whatever function that calls to print out a particular datatype. Seems to me like all the entries in the dataformat struct should just be a single character.

lrknox pushed a commit to lrknox/hdf5 that referenced this pull request Aug 10, 2023
@jhendersonHDF @lrknox
Make h5dump spacing consistent when printing VLEN datatype (HDFGroup#…
c3a5a54 
…3351) (HDFGroup#3352)
lrknox added a commit that referenced this pull request Aug 10, 2023
@lrknox
Merges from 1.14 Aug 4-9 (#3366)
0c1ad6c 
* Make h5dump spacing consistent when printing VLEN datatype (#3351) (#3352)

* Add Fortran H5ES module to deploy list (#3342)

* Fix for the bug exposed from running test/set_extent.c when selection I/O is enabled.  (#3319)

The test/set_extent.c is modified to test for selection I/O enabled.

* Merge Implementation of the mpio driver with selection I/O. (#3360)

* Work around a testphdf5 failure on Cray MPICH machines (#3361) (#3362)

* set H5_PAC_C_MAX_REAL_PRECISION default to 0 when cross compiling (#3365)
  with CMake to fix Fortran build failures.
@jhendersonHDF jhendersonHDF deleted the h5dump_vlen_1_14 branch August 31, 2023 19:20
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

@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

@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
No one assigned
Labels
Component - Tools Command-line tools like h5dump, includes high-level tools Merge Use this label when a PR is for a downstream merge Priority - 3. Low 🔽 Code cleanup, small feature change requests, etc. Type - Task Actions that don't fit into any other type category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jhendersonHDF @byrnHDF @derobins