Skip to content

Fix assertion on empty array case - #13942

Merged
gasche merged 1 commit into
ocaml:trunkfrom
OlivierNicole:fix-13939
Apr 8, 2025
Merged

gasche merged 1 commit into
ocaml:trunkfrom
OlivierNicole:fix-13939

Conversation

@OlivierNicole

Copy link
Copy Markdown
Contributor

Fixes #13939.

For consistency, I added an assertion in caml_floatarray_unsafe_set where it was absent before.

@gasche gasche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments:

  • Another approach would have been to move the assertion after the bound check. I like your current approach a bit more because it is explicit about the intended dynamic type of the input.

  • I have mixed feelings about silencing an assert failure (in debug mode) when calling unsafe_{get,set} on empty arrays, which is undefined behavior. On the other hand, by this reasoning we could also assert that the access is in-bounds in general, and we don't do this, so allowing accesses on 0-sized atoms is arguably consistent.

@gasche

gasche commented Apr 8, 2025

Copy link
Copy Markdown
Member

I'm not sure whether or not we wanted a Changes entry, but I was tempted to click "merge" so I added the label to skip it. Please feel free to just direct-push one if you forgot but think that one is useful.

@OlivierNicole

Copy link
Copy Markdown
Contributor Author

I just force-pushed an amended commit with the changelog updated.

@gasche
gasche merged commit 5df49ac into ocaml:trunk Apr 8, 2025
@OlivierNicole
OlivierNicole deleted the fix-13939 branch April 9, 2025 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Accessing an empty array can crash the debug runtime instead of raising

2 participants