Skip to content

Conversation

@cmb69
Copy link
Member

@cmb69 cmb69 commented Jul 25, 2024

This partially reverts 0956267, which introduced a type incompatibility where an int function is assigned to a zend_result function. That yields a level 1 C4133 warning on MSVC, and usually (e.g. in CI) level 1 warnings are elevated to errors, so the build fails.[1]

The PHP-8.3 branch and up are uneffected by this, so the upward merges should be empty.

[1] 0956267#r144587696


See https://github.com/php/php-src/actions/runs/10078923540/job/27864997876#step:5:897 for detailed info about the build failure.

This partially reverts 0956267, which
introduced a type incompatibility where an `int` function is assigned
to a `zend_result` function.  That yields a level 1 C4133 warning on
MSVC, and usually (e.g. in CI) level 1 warnings are elevated to errors,
so the build fails.[1]

The PHP-8.3 branch and up are uneffected by this, so the upward merges
should be empty.

[1] <php@0956267#r144587696>
@cmb69 cmb69 requested a review from Girgias as a code owner July 25, 2024 10:02
@cmb69 cmb69 requested review from morrisonlevi and removed request for Girgias July 25, 2024 10:02
@cmb69 cmb69 requested a review from Girgias July 25, 2024 10:02
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.

sigh It the function pointer issue thing again... I really dislike the fact that GCC and Clang don't care about this issue, MSVC is actually being sensible here.

And even more frustrating that there is no way to have this warn.....

Feel free to merge, and possibly even have a comment explaning why we have the warning for this version so we don't make this change again.

@cmb69
Copy link
Member Author

cmb69 commented Jul 26, 2024

Thank you! I've added a respective comment, and closed via af789af.

@cmb69 cmb69 closed this Jul 26, 2024
@cmb69 cmb69 deleted the cmb/session-c4133 branch July 26, 2024 13:00
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.

2 participants