Fix issue with invalid compound conversions performing no-op I/O #4166
Conversation
jhendersonHDF
added
Merge - To 1.14
Priority - 3. Low 🔽
jhendersonHDF
requested review from
lrknox,
derobins,
byrnHDF,
fortnern,
qkoziol,
vchoi-hdfgroup,
bmribler,
glennsong09,
mattjala and
brtnfld
as code owners
| hid_t compound_i32 = H5I_INVALID_HID; | ||
| #endif | ||
| #if H5_SIZEOF_INT64_T != 0 | ||
| hid_t compound_i64 = H5I_INVALID_HID; |
There was a problem hiding this comment.
Fixed size types are used here just to make sure that the optimizations in struct conversions around smaller or larger destination types don't potentially break this in the future
jhendersonHDF
force-pushed
the
compound_conv_empty_subset
branch
from
e615fbe to
5548a16
Compare
|
|
||
| TESTING("compound conversions where destination compound has no members from source compound"); | ||
|
|
||
| #if H5_SIZEOF_INT32_T != 0 |
There was a problem hiding this comment.
Why the ifdefs on type sizes? We demand C99 types and use them elsewhere in the library without protection.
There was a problem hiding this comment.
int8_t, 16, 32 and 64 are optional in C99, though I suspect they're fairly widely supported. It could be a problem on some systems though; this is just being cautious about it.
There was a problem hiding this comment.
The least and fast variants are guaranteed in C99 for 8, 16, 32, and 64, but the exact width types aren't.
There was a problem hiding this comment.
We use int32_t, etc. throughout the library. A hid_t ID is defined to be an int64_t. It's basically a requirement to build, or even use, HDF5. There's no need for the ifdefs here.
There was a problem hiding this comment.
Just because int64_t is defined doesn't guarantee that int16_t is, for example. I can remove the ifdefs if we think it's just visual noise, but something to think about. If those are requirements for the library, it may not be a bad idea to throw an error at configure time if one of the types we use directly is missing (if we don't already have that).
There was a problem hiding this comment.
I think int16_t is as well-established as int32_t and int64_t at this point.
|
Why is this considered an error? Is there an associated issue? I'd argue that the library is doing exactly what was asked of it. |
To me this seems like surprising behavior. While I agree the library is basically just doing what it's told, in my opinion it seems like this case would more often than not be user error that would be better to error on. I'm thinking of the case where a user may want to convert between two compounds of slightly different member types during write, say float->float16, but accidentally messed up the member name in the source compound slightly and is surprised when the dataset has fill data. It's definitely arguing semantics, but I'm not sure if we document anywhere what the expected behavior should be. I picked a behavior here, but am also fine with leaving the behavior as is, as long as we document it (or make that more prominent if we already have). |
|
There's definitely good arguments for both sides here. To Neil's point: we don't throw an error when an application ends up with a selection of 0 elements and performs I/O on it. On the other hand, it is easy to misspell a field name. |
|
Perhaps we could have a query-only property that provided the number of elements accessed for an I/O operation? |
|
Closing as it was decided that this should be the expected behavior and should be documented. This may be revisited in the future if we add a method for telling the library to check for possibly unexpected circumstances. |
No description provided.