Skip to content

Conversation

@TimWolla
Copy link
Member

@TimWolla TimWolla commented Jan 9, 2024

Note: Planning to merge this using “Rebase and merge”, to keep the original commits. Bundling it in the same PR for ease of review.

…_construct()

This is for consistency with xoshiro256**'s constructor.
Instead of hardcoding struct names, or even sizes, we can just determine the
actual size of the target structure using sizeof().
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

LGTM :)


if (seed_is_null) {
if (php_random_bytes_throw(&state->state, sizeof(php_random_uint128_t)) == FAILURE) {
if (php_random_bytes_throw(&state->state, sizeof(state->state)) == FAILURE) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming this is determined at compile time because the compiler knows the struct field type.

Copy link
Member Author

Choose a reason for hiding this comment

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

sizeof is always compile time.

Copy link
Member

Choose a reason for hiding this comment

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

That is technically incorrect sizeof of a VLA is a runtime construct. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL, but VLA are terrible anyway 😄

Copy link
Contributor

@zeriyoshi zeriyoshi left a comment

Choose a reason for hiding this comment

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

LGTM

@TimWolla TimWolla merged commit db68565 into php:master Jan 11, 2024
@TimWolla TimWolla deleted the engine-construct-cleanup branch January 11, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants