Skip to content

Conversation

@jorgsowa
Copy link
Contributor

Pre-allocated size of array for _php_gettimeofday and improved typing.

@jorgsowa jorgsowa requested review from bukka and derickr as code owners May 14, 2025 20:49
@jorgsowa jorgsowa changed the title ext/date: various optimizations ext/date: various optimizations of _php_gettimeofday May 14, 2025
Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

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

changes themselves are correct, optimisation"s" is only about the output though.

$get_as_float = false;

// Calling gettimeofday() with all possible arguments
var_dump( gettimeofday($get_as_float) );
Copy link
Member

Choose a reason for hiding this comment

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

Please don't change test cases at the same time as actual code changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't change the test case. It's only removal of unnecessary variables and comments. There is no functional change.

offset = timelib_get_time_zone_info(tp.tv_sec, get_timezone_info());

array_init(return_value);
array_init_size(return_value, 4);
Copy link
Member

Choose a reason for hiding this comment

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

Does this really improve things a lot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the same change as in recent PR #18382

Not much, but still something.

@jorgsowa
Copy link
Contributor Author

May this PR be merged?

@devnexen
Copy link
Member

devnexen commented May 31, 2025

If I may, I would suggest to revert the test changes (can be argued and done in a separate PR).

@jorgsowa
Copy link
Contributor Author

It makes completely no sense. Anyways, it's not important PR.

@jorgsowa jorgsowa closed this May 31, 2025
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