Skip to content

Split headers, part 2 #10657

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Feb 26, 2023
Merged

Split headers, part 2 #10657

merged 8 commits into from
Feb 26, 2023

Conversation

MaxKellermann
Copy link
Contributor

@MaxKellermann MaxKellermann commented Feb 21, 2023 •
edited
Loading

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

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.

@MaxKellermann
Copy link
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
Copy link
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
Copy link
Member

Girgias commented Feb 23, 2023

zend_uchar

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

@MaxKellermann
Copy link
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
Copy link
Member

Girgias commented Feb 23, 2023

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
Copy link
Contributor Author

MaxKellermann commented Feb 23, 2023 •
edited
Loading

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