Skip to content

Use consistent type for h on call zend_hash_index_* - #19255

Merged
Girgias merged 1 commit into
php:masterfrom
marc-mabe:consistent_type_call_zend_hash_index
Apr 6, 2026
Merged

Use consistent type for h on call zend_hash_index_*#19255
Girgias merged 1 commit into
php:masterfrom
marc-mabe:consistent_type_call_zend_hash_index

Conversation

@marc-mabe

Copy link
Copy Markdown
Contributor

zend_hash_index_* expects zend_ulong for the h argument but often this argument got casted to zend_long (instead of zend_ulong.

Additionally, most of the time, a pointer first got casted to (uintptr_t) before (zend_ulong) but sometimes a pointer got casted directly. I noticed this during my work of #19079 as it fails if the pointer size and zend_ulong differs.

@devnexen

Copy link
Copy Markdown
Member

changes look correct to me cc @nielsdos

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

The changes are right, although also kinda not necessary at this point.

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

Adding (uintptr_t) makes sense too eliminate warnings.

I'm not sure if changes between long/ulong make sense.
Probably, it would be better to use zend_long argument in first place, because we may work with negative indexes.

@marc-mabe

Copy link
Copy Markdown
Contributor Author

Adding (uintptr_t) makes sense too eliminate warnings.

I'm not sure if changes between long/ulong make sense. Probably, it would be better to use zend_long argument in first place, because we may work with negative indexes.

@dstogov
Modifying zend_hash_index_* functions to take zend_long over zend_ulong would modify the function signatures - that's why I avoided it. Also, it would be more work to do so and probably cast to (intptr_t) instead of (uintptr_t).

What do you think?

@dstogov

dstogov commented Sep 1, 2025

Copy link
Copy Markdown
Member

I didn't propose converting prototypes. I told that changes cast from zend_long to zend_ulong doesn't change anything. Anyway, this doesn't harm and the patch is good.

@Girgias
Girgias merged commit f4d9ed1 into php:master Apr 6, 2026
9 checks passed
adrian-enspired pushed a commit to adrian-enspired/php-src that referenced this pull request Aug 4, 2026
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.

5 participants