Skip to content

FPM: refactor fpm_php_get_string_from_table() to better match usage#11051

Merged
Girgias merged 2 commits into
php:masterfrom
Girgias:fpm-refactor-get-string-from-table
May 20, 2023
Merged

FPM: refactor fpm_php_get_string_from_table() to better match usage#11051
Girgias merged 2 commits into
php:masterfrom
Girgias:fpm-refactor-get-string-from-table

Conversation

@Girgias

@Girgias Girgias commented Apr 10, 2023

Copy link
Copy Markdown
Member

Pass in length of the key to improve the existance of the key check.

Feel free to close if you don't agree @bukka, but the returned char* is never used and passing in the length of the key allows performing the quick check of "do both string lengths match".

@Girgias
Girgias requested a review from bukka as a code owner April 10, 2023 13:43
Comment thread sapi/fpm/fpm/fpm_php.h Outdated

@bukka bukka 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.

Looks reasonable - just some minor comments

Comment thread sapi/fpm/fpm/fpm_status.c Outdated
Comment thread sapi/fpm/fpm/fpm_php.c Outdated

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.

I would rather leave this as a check as it's kind of external things from FPM PoV...

Comment thread sapi/fpm/fpm/fpm_php.h Outdated
Girgias added 2 commits April 11, 2023 11:30
Pass in length of the key to improve the existance of the key check
@Girgias
Girgias force-pushed the fpm-refactor-get-string-from-table branch from 3799dfa to 3360378 Compare April 11, 2023 10:43
@Girgias

Girgias commented Apr 11, 2023

Copy link
Copy Markdown
Member Author

Addressed the review comments, only other change was to move the declaration of the zend_string* inside the if branch as that's where it is only used.

@Girgias
Girgias requested a review from bukka May 5, 2023 15:15

@bukka bukka 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.

All looks good to me.

@Girgias
Girgias merged commit 548e061 into php:master May 20, 2023
@Girgias
Girgias deleted the fpm-refactor-get-string-from-table branch May 20, 2023 21:49
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.

2 participants