Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 H5Tdecode2, rename and deprecate H5Tdecode #5213
base: develop
Are you sure you want to change the base?
Add H5Tdecode2, rename and deprecate H5Tdecode #5213
Changes from 5 commits
b33ab75
55c1a68
6da7a5b
d941398
6981bc0
30730d4
b1c6e2d
a53b1ad
239c311
9b2e54a
fd8e1c2
cc0bbc4
b6bf81a
efc00ea
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing the issue, but I'm sorry, I made a mistake in the original comment. If encoded_buf is not NULL then buf_size will already be the size of the encoded buffer. You can safely use buf_size in H5Tdecode2 as you originally did. I'm sorry again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without having to call H5Tencode to get the size, that is. Checking buf_size is a good idea though. However, if you want to put a fix in DataType::close() to set encoded_buf to NULL then checking for NULL encoded_buf alone will be good enough. Question me if you see anything I said questionable. :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I was going to mention that there should be the corresponding JNI changes, but it seems there is no
_H5Tdecode
implementation currently. @byrnHDF We should either implement that or remove this function from being available.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big fan of an explicit comparison against the value 2 here. Using 2 occurrences of the
H5_IS_BUFFER_OVERFLOW
with a size of 1, for example, might be more verbose, but IMO is a lot clearer about what's going on.