Skip to content

Various improvement in documentation and a decoding function#6383

Open
bmribler wants to merge 7 commits into
HDFGroup:developfrom
bmribler:improve_error_checking
Open

Various improvement in documentation and a decoding function#6383
bmribler wants to merge 7 commits into
HDFGroup:developfrom
bmribler:improve_error_checking

Conversation

@bmribler
Copy link
Copy Markdown
Collaborator

- Improves documentation on the type size when creating/accessing a compound datatype with no predefined struct (GH issue HDFGroup#5371)
- Provides better description of the min_meta_perc and min_raw_perc arguments in the H5Pset_page_buffer_size() (GH issue HDFGroup#5711)
- Adds error checkings to an internal decoding function
Comment thread hl/src/H5TBpublic.h Outdated
* new records.
*
* \p type_size can be obtained with \c sizeof() if the data is stored
* stored in a predefined C struct, otherwise, use H5Tget_size() on
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

extra "stored" to be removed.

Comment thread hl/src/H5TBpublic.h Outdated
* by the identifier \p loc_id.
*
* \p type_size can be obtained with \c sizeof() if the data is stored
* stored in a predefined C struct, otherwise, use H5Tget_size() on
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

extra "stored" to be removed.

Comment thread hl/src/H5TBpublic.h Outdated
* specified by the identifier \p loc_id.
*
* \p type_size can be obtained with \c sizeof() if the data is stored
* stored in a predefined C struct, otherwise, use H5Tget_size() on
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

extra "stored" to be removed.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @vchoi-hdfgroup!

vchoi-hdfgroup
vchoi-hdfgroup previously approved these changes May 15, 2026
Comment thread src/H5Fsuper_cache.c
/* Validate addresses against stored_eof.
Skip for multi-file and split drivers which use relative/fractional addresses
that are not actual file offsets. */
if (udata->f->shared->lf->cls->value != H5_VFD_MULTI) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It seems a bit fragile to me to be checking for specific VFDs here, as it makes assumptions that may not hold for current or future VFDs.

Comment thread hl/src/H5TBpublic.h Outdated
* identifier loc_id.
*
* \p type_size can be obtained with \c sizeof() if the data is
* stored in a predefined C struct, otherwise, use H5Tget_size() on
Copy link
Copy Markdown
Collaborator

@jhendersonHDF jhendersonHDF May 19, 2026

Choose a reason for hiding this comment

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

I'm thinking this wording might be confusing depending on how the API functions are generally used. If you have a C struct type you can simply call sizeof() on it as mentioned here. But if you don't have a struct type, are you really going to create an HDF5 compound datatype just to get the size for it (and for H5TBmake_table to re-create the same type internally), or are you more likely just going to add together the sizes of each field directly to get a size? I would think the latter approach would make more sense.

Copy link
Copy Markdown
Collaborator Author

@bmribler bmribler May 19, 2026

Choose a reason for hiding this comment

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

Ah, my bad. I made a wrong assumption when I tried to improve that description following this discussion. Thanks for catching it, @jhendersonHDF!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To be triaged

Development

Successfully merging this pull request may close these issues.

4 participants