Fix assertion failure in H5S__copy_pnt_list() #5352
Conversation
jhendersonHDF
added
Priority - 3. Low 🔽
jhendersonHDF
requested review from
lrknox,
derobins,
byrnHDF,
fortnern,
qkoziol,
vchoi-hdfgroup,
bmribler,
glennsong09,
mattjala and
brtnfld
as code owners
jhendersonHDF
force-pushed
the
point_sel_copy_bug
branch
from
de1f40f to
293b777
Compare
|
Foiled again by the linkchecker |
jhendersonHDF
force-pushed
the
point_sel_copy_bug
branch
from
293b777 to
4ba5c1a
Compare
|
@qkoziol apologies on the timing; realized I should add a release note even though the issue is a minor one. |
|
|
||
| /* Allocate room for the head of the point list */ | ||
| if (NULL == (dst = H5FL_CALLOC(H5S_pnt_list_t))) | ||
| HGOTO_ERROR(H5E_DATASPACE, H5E_CANTALLOC, NULL, "can't allocate point list node"); | ||
|
|
||
| /* If source dataspace has no extent, just return new point list node */ | ||
| if (rank == 0) | ||
| HGOTO_DONE(dst); |
There was a problem hiding this comment.
@qkoziol I'm considering revising this. While it makes sense to me to return an empty point list node when the dataspace's extent has 0 dimensions (as we can't even tell how to copy the points over in that case), this causes problems elsewhere where the library expects space->select.sel_info.pnt_lst->head to be non-NULL. I have a fix for that as well, but I'm thinking of just having H5Scopy() / H5Sselect_copy() return failure for point and hyperslab selection dataspaces with a 0 dimension extent, since they both don't support H5S_SCALAR or H5S_NULL dataspaces and can only end up here when H5Sset_extent_none() is called on them. Thoughts?
There was a problem hiding this comment.
That would be fine. Another solution might be to have H5Sset_extent_none() clear the selection also (since you can't select anything within a "none" extent"). Generically, I would suggest reseting the selection any time that the extent's rank changes (also).
There was a problem hiding this comment.
That seems like a better solution to me. I suppose it makes sense to reset the selection to 'all' since that's what dataspaces start with?
There was a problem hiding this comment.
Agree, back to 'all' for all changes in rank would be consistent with dataspace creation.
No description provided.