Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Split headers, part 2#10657

Merged
Girgias merged 8 commits into
php:masterphp/php-src:masterfrom
CM4all:split_headersCM4all/php-src:split_headersCopy head branch name to clipboard
Feb 26, 2023
Merged

Split headers, part 2#10657
Girgias merged 8 commits into
php:masterphp/php-src:masterfrom
CM4all:split_headersCM4all/php-src:split_headersCopy head branch name to clipboard

Conversation

@MaxKellermann

@MaxKellermann MaxKellermann commented Feb 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

More like #10609 implementing the RFC vote https://wiki.php.net/rfc/include_cleanup to reduce header dependencies. This also allows converting lots of preprocessor macros to inline functions (which improves debugging - macros cannot be debugged, and macros are error prone). This wasn't possible previously because those macros could only be expanded after including a full set of headers, and due to circular header dependencies, these couldn't be made available just from zend_string.h.

@MaxKellermann MaxKellermann mentioned this pull request Feb 22, 2023
Merged
Girgias
Girgias reviewed Feb 23, 2023
View reviewed changes

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

Took a while to go through the commit list to see the point in the header splits.

Not a massive fan of having that single header for zend_uchar tho, seems rather pointless.

Comment thread Zend/zend_rc_debug.c Outdated
@MaxKellermann

MaxKellermann commented Feb 23, 2023

Copy link
Copy Markdown
Contributor Author

Not a massive fan of having that single header for zend_uchar tho, seems rather pointless.

Me neither. But zend_uchar living in zend_types.h means everything depends on zend_types.h, and in turn zend_types.h is not allowed to depend on anything, or else you're going circular.

zend_types.h sounds like the right name for a file providing basic types as zend_uchar, but zend_types.h contains so much more than basic types, and it's impossible to declutter it this way.

@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar
80356e4 
`zend_uchar` suggests that the value is an ASCII character, but here,
it's about very small integers.  This is misleading, so let's use a
C99 integer instead.

On all architectures currently supported by PHP, `zend_uchar` and
`uint8_t` are identical.  This change is only about code readability.
@MaxKellermann
Zend/zend_types.h: move zend_rc_debug to zend_rc_debug.h
0883b72 
`zend_rc_debug` is not a type and does not really belong in
`zend_types.h`; this allows using `ZEND_RC_MOD_CHECK()` without
including the huge `zend_types.h` header and allows decoupling
circular header dependencies.
@MaxKellermann
Zend/zend_rc_debug: convert ZEND_RC_MOD_CHECK() to function
913c9c9 
This allows using `ZEND_RC_MOD_CHECK()` without including any additional
headers.  Performance is not relevant here because this is a
debug-only feature.

The `zend_refcounted_h` forward declaration is necessary to break a
circular header dependency.
@MaxKellermann
Zend/zend_types.h: move IS_* to zend_type_code.h
365fd6f 
More decoupling of circular header dependencies.
@MaxKellermann
Zend/zend_type_code.h: convert to enum
a849b01
@MaxKellermann
Zend/zend_types.h: move zend_refcounted to zend_refcounted.h
e4c4d1d 
This is necessary for splitting `zend_types.h` further.
@MaxKellermann
Zend/zend_types.h: move zend_uchar.h to zend_char.h
a8614d9 
Prepare to fix the cyclic header dependency from `zend_string.h`.
@MaxKellermann
Zend/zend_types.h: move zend_string to zend_string.h
adee720 
It is now possible to include only `zend_string.h` without
`zend_types.h`.
@MaxKellermann

MaxKellermann commented Feb 23, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for merging #10621 - rebased this PR (dropping the first commit), fixed the coding style issue, and ready for "real" review now!

(As usual, I prefer to not squash for merging, or else this will leave a big messy patch in the git history. My PRs are consumed best as-is.)

@MaxKellermann MaxKellermann marked this pull request as ready for review February 23, 2023 15:04
@Girgias

Girgias commented Feb 23, 2023

Copy link
Copy Markdown
Member

zend_uchar

Doing a quick git grep, seems like most of the remaining usages of zend_uchar are in the MySQL ND codebase.

@MaxKellermann

MaxKellermann commented Feb 23, 2023

Copy link
Copy Markdown
Contributor Author

zend_types.h sounds like the right name for a file providing basic types as zend_uchar

Maybe, once all the complex stuff has been moved out of zend_types.h, we can move zend_uchar back there? Right now, moving it out solves a problem, but as I said, I agree.

@Girgias

Girgias commented Feb 23, 2023

Copy link
Copy Markdown
Member

Thanks for merging #10621 - rebased this PR (dropping the first commit), fixed the coding style issue, and ready for "real" review now!

(As usual, I prefer to not squash for merging, or else this will leave a big messy patch in the git history. My PRs are consumed best as-is.)

That one I'm not planning on squashing to keep the commits atomic, the previous ones were similar enough in what they were doing that I don't think squashing them is that harmful. :)

@MaxKellermann

MaxKellermann commented Feb 23, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Doing a quick git grep, seems like most of the remaining usages of zend_uchar are in the MySQL ND codebase.

And in the string stuff in zend_types.h which I'd like to move to a separate header, which is impossible because it would produce a circular dependency

And in zend_string.h which depends on zend_types.h, but moving string macros from zend_types.h to zend_string.h (where they really belong) produces a circular dependency.

@Girgias Girgias merged commit 02690fe into php:master Feb 26, 2023
@MaxKellermann MaxKellermann mentioned this pull request Feb 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Girgias Girgias Girgias left review comments

Assignees

No one assigned

Labels

Category: Engine SAPI: fpm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MaxKellermann @Girgias
Morty Proxy This is a proxified and sanitized view of the page, visit original site.