hdf5 icon indicating copy to clipboard operation
hdf5 copied to clipboard

Fix doc for H5Fget_access_plist

Open markcmiller86 opened this issue 10 months ago • 4 comments

I tried to find where in the code base is the true source for the documentation on H5Fget_access_plist() but was not able to.

There is no explicit verbiage there to indicate that the returned hid_t should be closed with a call to H5Pclose(...). However, there is such verbiage for H5Fget_create_plist().

Now, if it is always the case that any hid_t returned to caller must be closed, it may be sufficient to document that alone up front and then not worry about repeating that ad infinitum. But, since the docs do kinda sorta do that, I had the impression it was intetionally not included in the description of H5Fget_access_plist() maybe becase that was not needed or desired.

markcmiller86 avatar Feb 19 '25 01:02 markcmiller86

Somewhat related to this there is reference to the wrong method here (Should be referring to get method, not set method) Image

markcmiller86 avatar Feb 19 '25 01:02 markcmiller86

I tried to find where in the code base is the true source for the documentation on H5Fget_access_plist() but was not able to.

Most or all documentation for functions should be in the public header at this point. For H5Fget_access_plist() the documentation is at https://github.com/HDFGroup/hdf5/blob/develop/src/H5Fpublic.h#L690-L704, with the "Returns a file access property list identifier if successful; otherwise returns H5I_INVALID_HID" bit coming from our custom \hid_t{file access property list} command at https://github.com/HDFGroup/hdf5/blob/develop/doxygen/aliases#L44. We could add the "The creation property list identifier should be released with H5Pclose()." text, but it might need to be a new command since not all hid_t values returned from the library necessarily need to be closed, and not always with H5Pclose(). It was unintentional for the text to be left off, as the property list ID that is returned does need to be closed with H5Pclose(). I think a simple copy-paste in this case is easiest for now.

Somewhat related to this there is reference to the wrong method here (Should be referring to get method, not set method)

Thanks! We should fix that as well.

jhendersonHDF avatar Feb 19 '25 17:02 jhendersonHDF

I think the text "The creation property list identifier should be released with H5Pclose()." should be in the detail section, which is the case of some other functions, such as: H5Fcreate, or H5Dget_create_plist...

bmribler avatar Feb 19 '25 19:02 bmribler

I think the text "The creation property list identifier should be released with H5Pclose()." should be in the detail section, which is the case of some other functions, such as: H5Fcreate, or H5Dget_create_plist...

Yes, the point was about trying to make the text consistent everywhere, potentially by putting it behind a doxygen command. Developers would still likely copy-paste from a similar function and could miss places where the new command should be, but that's always going to be the case.

jhendersonHDF avatar Feb 19 '25 19:02 jhendersonHDF