Skip to content

zend_compiler, ...: use uint8_t instead of zend_uchar #10621

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 1 commit into from
Feb 23, 2023
Merged

zend_compiler, ...: use uint8_t instead of zend_uchar #10621

merged 1 commit into from
Feb 23, 2023

Conversation

MaxKellermann
Copy link
Contributor

@MaxKellermann MaxKellermann commented Feb 19, 2023 •
edited
Loading

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 uint_t are identical. This change is only about code readability.

(See #10597 (comment) for a discussion; @mvorisek)

TimWolla
TimWolla reviewed Feb 19, 2023
View reviewed changes
nikic
nikic requested changes Feb 19, 2023
View reviewed changes
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

To be safe on theoretical architectures which don't have uint8_t, this commit uses uint_least8_t instead, which is guaranteed to be supported.

We already make extensive use of uint8_t, you can assume it exists.

Especially your changes to the zval structure are outright incorrect if the type doesn't an 8 bit size.

@MaxKellermann
Copy link
Contributor Author

We already make extensive use of uint8_t, you can assume it exists.

Okay for me, it's just that @derickr felt it was important to point out to me that these types are optional. If that's not important at all for the rest of you, I can ignore his comment out it from now on.

@MaxKellermann
Copy link
Contributor Author

Changed to uint8_t.

mvorisek
mvorisek reviewed Feb 19, 2023
View reviewed changes
Zend/zend_stream.h Outdated
@@ -55,7 +55,7 @@ typedef struct _zend_file_handle {
} handle;
zend_string *filename;
zend_string *opened_path;
zend_uchar type; /* packed zend_stream_type */
uint8_t type; /* packed zend_stream_type */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uint8_t type; /* packed zend_stream_type */
uint8_t type; /* packed zend_stream_type */

MaxKellermann added a commit to CM4all/php-src that referenced this pull request Feb 20, 2023
@MaxKellermann
CODING_STANDARDS.md: establish C99 as the implementation language
81a1165 
PHP 8 switched to C99, but did not documented that anywhere.

After @derickr rejected a pull request on timelib
(derickr/timelib#141 (comment))
because my suggested change removed compile-time checks for
fixed-width integer types, pointing out that they are optional in the
C99 standard, @nikic disagreed with using `uint_least8_t` instead
(which is guaranteed to be available), stating that "We already make
extensive use of uint8_t, you can assume it exists"
(php#10621 (review)).

In order to avoid such confusion in the future, let's document this
architecture requirement.
@MaxKellermann MaxKellermann mentioned this pull request Feb 20, 2023
Merged
Girgias pushed a commit that referenced this pull request Feb 20, 2023
@MaxKellermann
CODING_STANDARDS.md: establish C99 as the implementation language (#1…
5bfd3fa 
…0631)

PHP 8 switched to C99, but did not documented that anywhere.

After @derickr rejected a pull request on timelib
(derickr/timelib#141 (comment))
because my suggested change removed compile-time checks for
fixed-width integer types, pointing out that they are optional in the
C99 standard, @nikic disagreed with using `uint_least8_t` instead
(which is guaranteed to be available), stating that "We already make
extensive use of uint8_t, you can assume it exists"
(#10621 (review)).

In order to avoid such confusion in the future, let's document this
architecture requirement.
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar
0bfc9fb 
`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
Copy link
Contributor Author

Rebased & fixed conflict.

Girgias
Girgias approved these changes Feb 21, 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.

LGTM now

@MaxKellermann MaxKellermann mentioned this pull request Feb 21, 2023
Merged
@MaxKellermann MaxKellermann changed the title zend_compiler, ...: use uint_least8_t instead of zend_uchar zend_compiler, ...: use uint8_t instead of zend_uchar Feb 22, 2023
@MaxKellermann MaxKellermann mentioned this pull request Feb 22, 2023
Merged
@Girgias Girgias merged commit d5c649b into php:master Feb 23, 2023
@MaxKellermann MaxKellermann deleted the replace_zend_uchar branch February 23, 2023 15:20
MaxKellermann added a commit to CM4all/php-src that referenced this pull request Mar 7, 2023
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
0a8b0d6 
`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 added a commit to CM4all/php-src that referenced this pull request Mar 7, 2023
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
1ce0baa 
`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 added a commit to CM4all/php-src that referenced this pull request Mar 14, 2023
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
74800f7 
`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 added a commit to CM4all/php-src that referenced this pull request Apr 12, 2023
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
e8b92cf 
`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 added a commit to CM4all/php-src that referenced this pull request May 16, 2023
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
9de7f14 
`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 added a commit to CM4all/php-src that referenced this pull request Jun 16, 2023
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
350ef95 
`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 added a commit to CM4all/php-src that referenced this pull request Jul 19, 2023
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
67706c9 
`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 added a commit to CM4all/php-src that referenced this pull request Aug 1, 2023
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
99a8ca4 
`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 added a commit to CM4all/php-src that referenced this pull request Aug 30, 2023
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
af38e5b 
`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 added a commit to CM4all/php-src that referenced this pull request Oct 2, 2023
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
b3e8eb2 
`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 added a commit to CM4all/php-src that referenced this pull request Nov 24, 2023
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
04fe396 
`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 added a commit to CM4all/php-src that referenced this pull request Jan 17, 2024
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
97224ec 
`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 added a commit to CM4all/php-src that referenced this pull request Jan 17, 2024
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
bf3349e 
`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 added a commit to CM4all/php-src that referenced this pull request Feb 23, 2024
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
89931a4 
`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 added a commit to CM4all/php-src that referenced this pull request Mar 15, 2024
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
306d632 
`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 added a commit to CM4all/php-src that referenced this pull request Apr 15, 2024
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
0f17ea4 
`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 added a commit to CM4all/php-src that referenced this pull request May 8, 2024
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
7c162b5 
`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 added a commit to CM4all/php-src that referenced this pull request Jun 5, 2024
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
f975280 
`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 added a commit to CM4all/php-src that referenced this pull request Jul 3, 2024
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
e8249cc 
`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 added a commit to CM4all/php-src that referenced this pull request Jul 30, 2024
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
9a52f9b 
`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 added a commit to CM4all/php-src that referenced this pull request Aug 28, 2024
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
d93f02d 
`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 added a commit to CM4all/php-src that referenced this pull request Oct 15, 2024
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
b41f5bd 
`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 added a commit to CM4all/php-src that referenced this pull request Oct 30, 2024
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
5f096da 
`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 added a commit to CM4all/php-src that referenced this pull request Nov 20, 2024
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
bee9ffd 
`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 added a commit to CM4all/php-src that referenced this pull request Jan 7, 2025
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
f5d7b00 
`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 added a commit to CM4all/php-src that referenced this pull request Mar 11, 2025
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
a0d93b6 
`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 added a commit to CM4all/php-src that referenced this pull request Apr 15, 2025
@MaxKellermann
zend_compiler, ...: use uint8_t instead of zend_uchar (php#10621)
7e56359 
`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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimWolla TimWolla TimWolla left review comments

@mvorisek mvorisek mvorisek left review comments

@nikic nikic nikic requested changes

@Girgias Girgias Girgias approved these changes

Assignees
No one assigned
Labels
Category: Engine Category: Optimizer Extension: com_dotnet Extension: ffi Extension: gmp Extension: intl Extension: mysqli Extension: opcache Extension: zend_test SAPI: phpdbg
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MaxKellermann @TimWolla @nikic @mvorisek @Girgias