Fix assertion failure in H5S__copy_pnt_list()

[jhendersonHDF]

Important

Fix assertion failure in H5S__copy_pnt_list() by adding a zero rank check and update tests.

• Bug Fix:
  • Fix assertion failure in H5S__copy_pnt_list() in H5Spoint.c by adding a check for zero rank and converting the assertion to a normal error check.
• Testing:
  • Add test_h5s_bug4() in th5s.c to test copying a dataspace with a point selection after extent reset with H5Sset_extent_none().
• Documentation:
  • Update RELEASE.txt to document the fix for the assertion failure in H5S__copy_pnt_list().

^{This description was created by for d9e2032. You can customize this summary. It will automatically update as commits are pushed.}

[jhendersonHDF]

Foiled again by the linkchecker

[jhendersonHDF]

@qkoziol apologies on the timing; realized I should add a release note even though the issue is a minor one.

[jhendersonHDF]

@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?

[qkoziol]

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).

[jhendersonHDF]

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?

[qkoziol]

Agree, back to 'all' for all changes in rank would be consistent with dataspace creation.

[brtnfld]

@ellipsis-dev, review PR.

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to d9e2032 in 1 minute and 58 seconds. Click for details.

• Reviewed 115 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. test/th5s.c:1689

• Draft comment:
  Test code in H5S_check_encoding uses hard-coded buffer offsets (e.g. buf[35] and buf[39]) to verify the encoded version and encoded size. Consider defining named constants or adding detailed comments, as these magic numbers are brittle if the encoding format changes.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_0axEUbe3fKUn9nQg

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

In test_h5s_bug4, the CHECK() call is mistakenly checking 'space_id' rather than the returned 'space_copy_id' from H5Scopy. Change it to CHECK(space_copy_id, H5I_INVALID_HID, "H5Scopy").

Suggested change

	CHECK(space_id, H5I_INVALID_HID, "H5Scopy");
	CHECK(space_copy_id, H5I_INVALID_HID, "H5Scopy");

[jhendersonHDF]

Will address with a different solution later