Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] bug 54547

Posted by Stas Malyshev 
Stas Malyshev
[PHP-DEV] bug 54547
May 13, 2012 04:50AM
Hi!

I know this was discussed a number of times here, but just to bring it
to a conclusion - I intend to apply patch in the bug report - which
removes conversion for strings that do not convert to integers - to 5.4.
If anybody sees anything that breaks because of this please tell. Not
sure what about 5.3 - Johannes, could you please comment?
I've run the tests and the patch does not seem to cause any breakage.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Gustavo Lopes
Re: [PHP-DEV] bug 54547
May 13, 2012 01:00PM
On Sun, 13 May 2012 04:39:33 +0200, Stas Malyshev <[email protected]>
wrote:

> I know this was discussed a number of times here, but just to bring it
> to a conclusion - I intend to apply patch in the bug report - which
> removes conversion for strings that do not convert to integers - to 5.4.
> If anybody sees anything that breaks because of this please tell. Not
> sure what about 5.3 - Johannes, could you please comment?
> I've run the tests and the patch does not seem to cause any breakage.

I should point out that "remove[ing] conversion for strings that do not
convert to integers" (let's call it proposition A) is not exactly what the
patch does. In addition to that condition, one other is required:

* The floats a and b the strings convert to are such that a - b == 0.0 (B)

It's implemented as

if (oflow1 != 0 && oflow1 == oflow2 && dval1 - dval2 == 0.) { ... }

This is irrelevant for == or != comparisons. Let's call C "strings are
equal in the memcmp() sense". If only A is evaluated, then if ~A, the
result of the comparison is the value C, i.e., if strings do not convert
to integers, the result of the comparison is the result of memcmp(). Also
under ~A, if we add B into the mix, we have the following results:

B ~B
C 1 can't occur
~C 0 0

But for the comparisons >, <, etc. the result may be surprising. Under ~A
&& ~B && ~C, using only the first condition results in a memcmp()
comparison, while under the two comparisons results in the floats being
compared. See:

$ php -r 'var_dump(strcmp(" 9223372036854775809",
"-9223372036854775808"));'
int(-1) (str1 is less than str2)
$ php -r 'var_dump((float)" 9223372036854775809" <
(float)"-9223372036854775808");'
bool(false)

So the float comparison behavior under ~B (what's in the patch) may seem
more desirable because it preserves the numerical comparison when possible
(and we don't have to add leading whitespace and zeros to the mix,
strcmp("9, "11") returns 1). Until you realize it's alternating between
two behaviors depending on whether B or ~B. So:

"9223372036854775809" < " 09443372036854775809" (true, -- floats differ,
compare as float)
"9223372036854775809" < " 09223372036854775810" (false -- floats are the
same, memcmp)

In both cases (incorporating the test for B or not), there's no escaping a
discontinuity in behavior, unless we revert not to memcmp() but to a
custom string comparison function that strips whitespace and leading
zeros, compares the size of the input and finally calls memcpy().

--
Gustavo Lopes

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Stas Malyshev
Re: [PHP-DEV] bug 54547
May 13, 2012 11:50PM
Hi!

> So the float comparison behavior under ~B (what's in the patch) may seem
> more desirable because it preserves the numerical comparison when possible
> (and we don't have to add leading whitespace and zeros to the mix,
> strcmp("9, "11") returns 1). Until you realize it's alternating between
> two behaviors depending on whether B or ~B. So:
>
> "9223372036854775809" < " 09443372036854775809" (true, -- floats differ,
> compare as float)
> "9223372036854775809" < " 09223372036854775810" (false -- floats are the
> same, memcmp)

I understand this may not be ideal but I really see this as very narrow
use case - if you really want to compare strings in lexicographical
order, you just should use strcmp. So I think this fixes common cases
where people have high WTF factor, and if we later have better idea on
how to fix all the cases we could amend it further. But I'm not sure
adding more magic to the mix (i.e. doing special comparisons, etc.) is
going to be better - it will only make it harder for people to understand.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Hartmut Holzgraefe
Re: [PHP-DEV] bug 54547
June 15, 2012 03:00AM
Too late to raise this now anyway, but ...

On 13.05.2012 04:39, Stas Malyshev wrote:
> I know this was discussed a number of times here, but just to bring it
> to a conclusion - I intend to apply patch in the bug report - which
> removes conversion for strings that do not convert to integers - to 5.4.
> If anybody sees anything that breaks because of this please tell.

"12345678901234567890" == "12345678901234567890.0"

used to be true, is now false ... i'd still say that's ok though as
it is a case of "never compare floats for equality" here, now that
the decimal clearly says that at least the right side is supposed
to be float, not integer ...

--
hartmut


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Tjerk Anne Meesters
Re: [PHP-DEV] bug 54547
June 15, 2012 04:20AM
On Fri, Jun 15, 2012 at 8:56 AM, Hartmut Holzgraefe
<[email protected]> wrote:
> Too late to raise this now anyway, but ...
>
> On 13.05.2012 04:39, Stas Malyshev wrote:
>> I know this was discussed a number of times here, but just to bring it
>> to a conclusion - I intend to apply patch in the bug report - which
>> removes conversion for strings that do not convert to integers - to 5.4.
>> If anybody sees anything that breaks because of this please tell.
>
> "12345678901234567890" == "12345678901234567890.0"
>
> used to be true, is now false ... i'd still say that's ok though as
> it is a case of "never compare floats for equality" here, now that
> the decimal clearly says that at least the right side is supposed
> to be float, not integer ...

I don't think this is a case of "never compare floats for equality", as:

12345678901234567890.0 == 12345678901234567890.1

The number is simply too big, even for a float, to handle reliably. In fact

number_format(12345678901234567890.0, 1, '', '') == '123456789012345671680'

>
> --
> hartmut
>
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>



--
--
Tjerk

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
OISHI Kazuo
Re: [PHP-DEV] bug 54547
June 15, 2012 07:50AM
>> I know this was discussed a number of times here, but just to bring it
>> to a conclusion - I intend to apply patch in the bug report - which
>> removes conversion for strings that do not convert to integers - to 5.4.
>> If anybody sees anything that breaks because of this please tell.
>
> "12345678901234567890" == "12345678901234567890.0"
>
> used to be true, is now false ... i'd still say that's ok though as
> it is a case of "never compare floats for equality" here, now that
> the decimal clearly says that at least the right side is supposed
> to be float, not integer ...

In addition to == operator, >, <, >=, and <= operators are influenced.

And, hexdecimal format for big number is now case-sensitive.

http://www.mail-archive.com/[email protected]/msg58789.html

--
OISHI Kazuo


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Christopher Jones
Re: [PHP-DEV] bug 54547
June 21, 2012 08:20PM
On 06/14/2012 10:42 PM, OISHI Kazuo wrote:
>>> I know this was discussed a number of times here, but just to bring it
>>> to a conclusion - I intend to apply patch in the bug report - which
>>> removes conversion for strings that do not convert to integers - to 5.4.
>>> If anybody sees anything that breaks because of this please tell.
>>
>> "12345678901234567890" == "12345678901234567890.0"
>>
>> used to be true, is now false ... i'd still say that's ok though as
>> it is a case of "never compare floats for equality" here, now that
>> the decimal clearly says that at least the right side is supposed
>> to be float, not integer ...
>
> In addition to == operator, >, <, >=, and <= operators are influenced.
>
> And, hexdecimal format for big number is now case-sensitive.
>
> http://www.mail-archive.com/[email protected]/msg58789.html
>

Can you add some phpt tests for all the cases you've raised?

Chris

--
christopher.jones@oracle.com
http://twitter.com/#!/ghrd



--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
OISHI Kazuo
Re: [PHP-DEV] bug 54547
June 22, 2012 09:20AM
Hi,

>> In addition to == operator, >, <, >=, and <= operators are influenced.
>>
>> And, hexdecimal format for big number is now case-sensitive.
>>
>> http://www.mail-archive.com/[email protected]/msg58789.html
>
> Can you add some phpt tests for all the cases you've raised?

This is the phpt. All tests passed in PHP 5.4.3 but failed in 5.4.4.
(I don't know how I can add this phpt into tests.)

==========================================================
--TEST--
Bug #62097: New behavior of string == has a compatibility problem (2)
--FILE--
<?php
var_dump("09223372036854775808" == "9223372036854775808");
var_dump(" 9223372036854775808" == "9223372036854775808");
var_dump("12345678901234567890.0" == "12345678901234567890");
var_dump("12345678901234567890e1" == "123456789012345678900");
var_dump("12345678901234567890e1" == "12345678901234567890E1");
var_dump("0xffffffffffffffff" == "0xFFFFFFFFFFFFFFFF");
var_dump("0xffffffffffffffff" > "0xFFFFFFFFFFFFFFFF");
var_dump("0xffffffffffffffff" <= "0xFFFFFFFFFFFFFFFF");

--EXPECT--
bool(true)
bool(true)
bool(true)
bool(true)
bool(true)
bool(true)
bool(false)
bool(true)
==========================================================

--
OISHI Kazuo


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Christopher Jones
Re: [PHP-DEV] bug 54547
June 27, 2012 02:20AM
On 06/22/2012 12:08 AM, OISHI Kazuo wrote:
> Hi,
>
>>> In addition to == operator, >, <, >=, and <= operators are influenced.
>>>
>>> And, hexdecimal format for big number is now case-sensitive.
>>>
>>> http://www.mail-archive.com/[email protected]/msg58789.html
>>
>> Can you add some phpt tests for all the cases you've raised?
>
> This is the phpt. All tests passed in PHP 5.4.3 but failed in 5.4.4.
> (I don't know how I can add this phpt into tests.)
>
> ==========================================================
> --TEST--
> Bug #62097: New behavior of string == has a compatibility problem (2)
> --FILE--
> <?php
> var_dump("09223372036854775808" == "9223372036854775808");
> var_dump(" 9223372036854775808" == "9223372036854775808");
> var_dump("12345678901234567890.0" == "12345678901234567890");
> var_dump("12345678901234567890e1" == "123456789012345678900");
> var_dump("12345678901234567890e1" == "12345678901234567890E1");
> var_dump("0xffffffffffffffff" == "0xFFFFFFFFFFFFFFFF");
> var_dump("0xffffffffffffffff" > "0xFFFFFFFFFFFFFFFF");
> var_dump("0xffffffffffffffff" <= "0xFFFFFFFFFFFFFFFF");
>
> --EXPECT--
> bool(true)
> bool(true)
> bool(true)
> bool(true)
> bool(true)
> bool(true)
> bool(false)
> bool(true)
> ==========================================================
>

Does this need an architecture specific SKIPIF? See the mention of
PHP_INT_SIZE on http://qa.php.net/write-test.php

I don't know whether to suggest adding an XFAIL for 5.4, or whether to
suggest changing the expected output. This would depend on whether
there is likely to be any code change to 5.4 or not.

Chris

--
christopher.jones@oracle.com
http://twitter.com/#!/ghrd



--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
OISHI Kazuo
Re: [PHP-DEV] bug 54547
June 27, 2012 06:10AM
> Does this need an architecture specific SKIPIF? See the mention of
> PHP_INT_SIZE on http://qa.php.net/write-test.php

Like this?
===============================================================
--SKIPIF--
<?php
if (PHP_INT_SIZE != 8) die("skip this test is for 64bit platforms only");
?>
===============================================================


I'm afraid I may miss your point.

>>> Can you add some phpt tests for all the cases you've raised?

Could you tell me your intention of this question?


> I don't know whether to suggest adding an XFAIL for 5.4, or whether to
> suggest changing the expected output. This would depend on whether
> there is likely to be any code change to 5.4 or not.

Sorry, I cannot understand what you say, but PHP code changes
from 5.4.3 to 5.4.4 for #54547 and #62097 are:

https://github.com/php/php-src/commit/9344bf193c6e35c8706923953f3e63bb01cc05ed
https://github.com/php/php-src/commit/acd711685a592c52be200e248154283c6c49c9f8

zendi_smart_strcmp() and is_numeric_string[_ex]() are changed.

--
OISHI Kazuo

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Christopher Jones
Re: [PHP-DEV] bug 54547
June 27, 2012 08:30PM
On 06/26/2012 09:06 PM, OISHI Kazuo wrote:
>> Does this need an architecture specific SKIPIF? See the mention of
>> PHP_INT_SIZE on http://qa.php.net/write-test.php
>
> Like this?
> ===============================================================
> --SKIPIF--
> <?php
> if (PHP_INT_SIZE != 8) die("skip this test is for 64bit platforms only");
> ?>
> ===============================================================
>
>

Yes - if your test is for 64 bit platforms. If other conditions
affect the output (endianess etc?) then you should add other tests
to the SKIPIF section.

Can you also create another PHPT file for 32 bit platforms, if there
isn't one already?

> I'm afraid I may miss your point.
>
>>>> Can you add some phpt tests for all the cases you've raised?
>
> Could you tell me your intention of this question?

So that next time this code changes any breakage is obvious.

>
>
>> I don't know whether to suggest adding an XFAIL for 5.4, or whether to
>> suggest changing the expected output. This would depend on whether
>> there is likely to be any code change to 5.4 or not.
>
> Sorry, I cannot understand what you say, but PHP code changes
> from 5.4.3 to 5.4.4 for #54547 and #62097 are:
>
> https://github.com/php/php-src/commit/9344bf193c6e35c8706923953f3e63bb01cc05ed
> https://github.com/php/php-src/commit/acd711685a592c52be200e248154283c6c49c9f8
>
> zendi_smart_strcmp() and is_numeric_string[_ex]() are changed.
>

Any test you create for PHP 5.4 need to either:

1. Pass, or
2. have an XFAIL section

I don't know whether or not PHP 5.4 is going to be changed. If it is not
going to be changed, then you should do #1.

Chris

--
christopher.jones@oracle.com
http://twitter.com/#!/ghrd



--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Oishi Kazuo
Re: [PHP-DEV] bug 54547
June 28, 2012 04:10AM
> So that next time this code changes any breakage is obvious.

Next time?

I had reported breakage for PHP 5.4.4 RC, but it "wont fix" and PHP
5.4.4 was released.
https://bugs.php.net/bug.php?id=62097

I think the breakage exists in current version (PHP 5.4.4).


> Any test you create for PHP 5.4 need to either:
>
> 1. Pass, or
> 2. have an XFAIL section
>
> I don't know whether or not PHP 5.4 is going to be changed. If it is not
> going to be changed, then you should do #1.

All my tests are passed at previous version (PHP 5.4.3), and are failed
at current version (PHP 5.4.4). Behavior of PHP 5.4 WAS changed.

I intend to show evidence of breakage (backward incompatibility)
in PHP 5.4.4, not intend to write test code to be passed on PHP 5.4.4.

--
Oishi Kazuo

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sorry, only registered users may post in this forum.

Click here to login