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

Add other types and full type to enum/str/vlen dataformat for structblock[begin/end] #3353

Merged
merged 3 commits into from
Aug 24, 2023
Merged

Add other types and full type to enum/str/vlen dataformat for structblock[begin/end] #3353

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4f735a2
Deprecate use of enum/str/vlen dataformat for structblock{begin/end]
byrnHDF Aug 4, 2023
5dd0771
Change dump struct to allow container types to be changed
byrnHDF Aug 9, 2023
9cb4198
Committing clang-format changes
github-actions[bot] Aug 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions tools/lib/h5tools_dump.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,14 +149,14 @@ const h5tools_dump_header_t h5tools_standardformat = {
"}", /*extlinkblockend */
"{", /*udlinkblockbegin */
"}", /*udlinkblockend */
"{", /*strblockbegin */
"}", /*strblockend */
"{", /*enumblockbegin */
"}", /*enumblockend */
"{", /*DO NOT USE strblockbegin */
"}", /*DO NOT USE strblockend */
"{", /*DO NOT USEenumblockbegin */
"}", /*DO NOT USEenumblockend */
Copy link
Member

Choose a reason for hiding this comment

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

Should have a space after DO NOT USE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yes!

"{", /*structblockbegin */
"}", /*structblockend */
"{", /*vlenblockbegin */
"}", /*vlenblockend */
"{", /*DO NOT USEvlenblockbegin */
"}", /*DO NOT USEvlenblockend */
"{", /*subsettingblockbegin */
"}", /*subsettingblockend */
"(", /*startblockbegin */
Expand Down Expand Up @@ -2236,7 +2236,7 @@ h5tools_print_datatype(FILE *stream, h5tools_str_t *buffer, const h5tool_format_
is_vlstr = H5Tis_variable_str(tmp_type);

curr_pos = ctx->cur_column;
h5tools_str_append(buffer, "H5T_STRING %s", h5tools_dump_header_format->strblockbegin);
h5tools_str_append(buffer, "H5T_STRING %s", h5tools_dump_header_format->structblockbegin);
h5tools_render_element(stream, info, ctx, buffer, &curr_pos, (size_t)ncols, (hsize_t)0,
(hsize_t)0);

Expand Down Expand Up @@ -2415,7 +2415,7 @@ h5tools_print_datatype(FILE *stream, h5tools_str_t *buffer, const h5tool_format_
if (H5Tclose(tmp_type) < 0)
H5TOOLS_ERROR((-1), "H5Tclose failed");

h5tools_str_append(buffer, "%s", h5tools_dump_header_format->strblockend);
h5tools_str_append(buffer, "%s", h5tools_dump_header_format->structblockend);
break;

case H5T_BITFIELD:
Expand Down Expand Up @@ -2532,7 +2532,7 @@ h5tools_print_datatype(FILE *stream, h5tools_str_t *buffer, const h5tool_format_
if ((super = H5Tget_super(type)) < 0)
H5TOOLS_THROW((-1), "H5Tget_super failed");

h5tools_str_append(buffer, "H5T_ENUM %s", h5tools_dump_header_format->enumblockbegin);
h5tools_str_append(buffer, "H5T_ENUM %s", h5tools_dump_header_format->structblockbegin);
h5tools_render_element(stream, info, ctx, buffer, &curr_pos, (size_t)ncols, (hsize_t)0,
(hsize_t)0);
ctx->indent_level++;
Expand All @@ -2556,22 +2556,22 @@ h5tools_print_datatype(FILE *stream, h5tools_str_t *buffer, const h5tool_format_
ctx->need_prefix = TRUE;

h5tools_str_reset(buffer);
h5tools_str_append(buffer, "%s", h5tools_dump_header_format->enumblockend);
h5tools_str_append(buffer, "%s", h5tools_dump_header_format->structblockend);

break;

case H5T_VLEN:
if ((super = H5Tget_super(type)) < 0)
H5TOOLS_THROW((-1), "H5Tget_super failed");

h5tools_str_append(buffer, "H5T_VLEN %s ", h5tools_dump_header_format->vlenblockbegin);
h5tools_str_append(buffer, "H5T_VLEN %s ", h5tools_dump_header_format->structblockbegin);

h5tools_print_datatype(stream, buffer, info, ctx, super, TRUE);

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->structblockend);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these not defined exactly the same? Looks like the idea was to make it easy to change the graphical delimiter for different types. With this change it's all or nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only three of the types are in the struct, this is more consistently applied.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd assume that's because it doesn't make sense to have a begin and end block for building block types like H5T_NATIVE_INT. This may be consistent but it seems like it's taking away an easy way to change how the output looks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but only three datatypes are available [str, enum and vlen]!
The others must be changed in the function code.
So this makes the usage consistent. The better way would be to allow all datatypes to be changed. That will change program design, this PR doesn't .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not proposing the absolute way it should be, only making the current state consistent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, but only three datatypes are available [str, enum and vlen]!

There's also opaque and compound that currently use structblockbegin and structblockend. I think it's a little odd to use structblockbegin for opaque types, but it appears that's because the DDL for an opaque type is a pseudo-structure since it includes the opaque tag:

DATATYPE  H5T_OPAQUE {
  OPAQUE_TAG "test opaque type";
}

I would've at least have created an opaqueblockbegin and opaqueblockend rather than reusing structblockbegin since the "struct" there was clearing referring to use with compound datatypes, but it is what it is.

The better way would be to allow all datatypes to be changed. That will change program design, this PR doesn't.

What I'm saying is there isn't anything to change for other datatypes because they aren't structured types, they're basic building block types. In the DDL for an 8-bit little-endian integer, you just get DATATYPE H5T_STD_I8LE, which looks reasonable to me and makes sense why there isn't an intblockbegin/intblockend. I actually think moving all these types to using only structblockbegin/structblockend is actually less consistent since to me it again seems clear that "struct" refers to compound datatypes only.

If we're going to remove the other begin/end fields it would be nice to just remove them from the struct, but it's unfortunate that they're part of the public API in the tools library (though that seems to me like another argument for not changing which fields are being used, even if they're all just mapped to {/}).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry I was unclear, I am thinking about the datatypes in the function switch statements where the original vlen change was made. Each use of structblock is the same "H5T_XXXX %s" printf. I would move those to the struct definines so programs could change those instances.


break;

Expand Down