Skip to content

Conversation

TomA-R
Copy link
Contributor

@TomA-R TomA-R commented Oct 19, 2025

Updates the IS_DOUBLE branch to use Z_DVAL_P(from) instead of Z_LVAL_P(from). The old code was reading from the integer slot, which isn’t defined for doubles and could return garbage.

The new logic follows PHP's own casting rules: 0.0 and -0.0 are false, everything else (1.5, INF, -INF, NAN) is true.

Added a PHPT test that fails with the old code but passes with this change.

No API changes, just correctness and consistency with PHP itself.

@TomA-R TomA-R requested a review from a team as a code owner October 19, 2025 00:37
@TomA-R TomA-R requested review from haberman and removed request for a team October 19, 2025 00:37
Copy link
Contributor

@honglooker honglooker left a comment

Choose a reason for hiding this comment

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

Looking good! Mind adding a copyright blurb?

We were reading floats like integers when converting to bool, which made -0.0 incorrectly evaluate to true. Using the proper float accessor fixes this and matches PHP: 0.0 and -0.0 are false; any non-zero float is true.
@TomA-R TomA-R force-pushed the php-double-bool-cast branch from 78d7f92 to 7e99380 Compare October 21, 2025 01:28
public function testBoolFromDoubleArrayConstructor()
{
$m = new TestMessage(['optional_bool' => -0.0]);
$this->assertFalse($m->getOptionalBool());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fails on main and passes on this branch (effectively proving the effectiveness of the fix). Same goes for line 533

@github-actions github-actions bot added untriaged auto added to all issues by default when created. and removed wait for user action labels Oct 21, 2025
Copy link
Contributor

@honglooker honglooker left a comment

Choose a reason for hiding this comment

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

Nice. Thank you!

@honglooker honglooker added 🅰️ safe for tests Mark a commit as safe to run presubmits over and removed untriaged auto added to all issues by default when created. labels Oct 21, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants