Skip to content

Conversation

marc-mabe
Copy link
Contributor

@marc-mabe marc-mabe marked this pull request as ready for review July 8, 2025 13:08
@marc-mabe marc-mabe force-pushed the bzdecompress-size-check branch from 1614151 to 37e3c9e Compare August 28, 2025 11:47
@marc-mabe
Copy link
Contributor Author

@Girgias ping you here as you have the most commits for this ext in the last year

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.

This does look reasonable to me, but maybe @cmb69 has better input about the Window's specific code that use to exist?

@cmb69
Copy link
Member

cmb69 commented Aug 28, 2025

Thanks for the PR! The code seems to be improvable, indeed.

However, why don't we use uint64_t instead of unsigned long long? From .total_out_hi32 and bzs.total_out_lo32 it's pretty clear that we have to deal with 64bit integers, anyway. Also, I'm not sure that the preprocessor guards are really relevant (performance wise), but if we keep them, we might better use https://github.com/php/php-src/blob/master/Zend/zend_range_check.h (either something suitable is already there, or we can add something).

@marc-mabe
Copy link
Contributor Author

Thanks for the PR! The code seems to be improvable, indeed.

However, why don't we use uint64_t instead of unsigned long long? From .total_out_hi32 and bzs.total_out_lo32 it's pretty clear that we have to deal with 64bit integers, anyway. Also, I'm not sure that the preprocessor guards are really relevant (performance wise), but if we keep them, we might better use https://github.com/php/php-src/blob/master/Zend/zend_range_check.h (either something suitable is already there, or we can add something).

@cmb69 true, I updated the PR in favor of uint64_t and removed the preprocessor checks. This may leave us with an impossible condition but my guess is that this would get optimized out anyway. Also added an UNEXPECTED here.

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.

3 participants