Skip to content

ext/phar: Readability Improvements in phar_fancy_stat()#21865

Merged
TimWolla merged 1 commit intophp:masterfrom
LamentXU123:opt
May 4, 2026
Merged

ext/phar: Readability Improvements in phar_fancy_stat()#21865
TimWolla merged 1 commit intophp:masterfrom
LamentXU123:opt

Conversation

@LamentXU123
Copy link
Copy Markdown
Contributor

This patch removes the temporary key array and uses ZEND_STRL(...) directly for each fixed key instead. This avoid repeated strlen() calls and also reduce code complexity.

@devnexen
Copy link
Copy Markdown
Member

devnexen commented Apr 24, 2026

I would say yes even though the benefit is really more the readability improvement more than "saving strlen calls", nowadays strlen(str)/sizeof(str)-1 the former still use __builtin_strlen behind the scene with compile time strings like here.

@LamentXU123
Copy link
Copy Markdown
Contributor Author

Test failures are unrelated :/

Copy link
Copy Markdown
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

I agree with David's comment. Can you adjust the commit message and PR title accordingly?

@LamentXU123 LamentXU123 changed the title ext/phar: Avoid repeated strlen() calls in phar_fancy_stat() ext/phar: Readability Improvements in phar_fancy_stat() Apr 27, 2026
@Girgias
Copy link
Copy Markdown
Member

Girgias commented Apr 29, 2026

Can you rebase the PR on master now that CI should be fixed.

@LamentXU123
Copy link
Copy Markdown
Contributor Author

Can you rebase the PR on master now that CI should be fixed.

Ah the CI is fixed now :)

@Girgias
Copy link
Copy Markdown
Member

Girgias commented May 4, 2026

Please rebase and force push rather than merging master, this adds random commits that make it hard to review.

@LamentXU123
Copy link
Copy Markdown
Contributor Author

Ah ok now it works. Its late night last time and I'm kind of stupid that I forgot to check it.. Anyway, now its clean :)

@TimWolla TimWolla merged commit eba954d into php:master May 4, 2026
19 checks passed
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.

4 participants