Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:44093
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain realplain.com from 209.151.69.1 cause and error)
Message-ID: <DECEF3A51CE54B85A8D7232E76F5E904@pc1>
To: <internals@lists.php.net>
Cc: "Dmitry Stogov" <dmitry@zend.com>,
	"Stas Malyshev" <stas@zend.com>,
	"Lukas Kahwe Smith" <mls@pooteeweet.org>,
	=?iso-8859-1?Q?Johannes_Schl=FCter?= <johannes@php.net>
References: <EB466381915442CFBBA6D6C9BCCB5392@pc1> <49D468CA.5050200@zend.com> <F29D36280ED8458AA3E2B3FA6BA94164@pc1> <49D65752.6020207@zend.com> <B653F7D59B1D47A6A303C65F48B9714D@pc1>
Date: Fri, 29 May 2009 15:50:00 -0500
MIME-Version: 1.0
Content-Type: text/plain;
	format=flowed;
	charset="iso-8859-1";
	reply-type=response
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] Re: [PATCH] double to long conversion change
From: php_lists@realplain.com ("Matt Wilmas")

Hi all,

----- Original Message -----
From: "Matt Wilmas"
Sent: Friday, April 10, 2009

> Hi Dmitry,
>
> I finally updated the patches:
> http://realplain.com/php/dval_to_lval.diff
> http://realplain.com/php/dval_to_lval_5_3.diff

The patches were updated again, against current CVS (no changes by me).

After some off- and on-list discussion weeks ago, it sounds like (from Stas) 
these changes are acceptable to restore float->int conversion to the 
long-standing behavior that exists through PHP 5.2 (for most 
users/platforms), as long as the necessary tests were updated, which I've 
done for ones I checked.  At least I haven't heard any negative feedback 
since my final proposal? :-)

Patches for test updates:
http://realplain.com/php/dval_to_lval_tests.diff
http://realplain.com/php/dval_to_lval_tests_5_3.diff

ext/standard/tests/url/parse_url_variation_002.phpt was split into 32- and 
64-bit versions again (like 5.2).  If other failing tests are found, let me 
know.

I guess I could go ahead and commit the changes next week if there aren't 
objections...  Was wondering about two minor parts, explained previously, OK 
to keep?:

1) Configure check for whether just a simple (long) cast gives the desired 
result, to save runtime range checking.  (Not sure how many platforms would 
pass this, other than Windows' VC6.)
2) The 'L' conversion specifier for zend_parse_parameters() (I've updated 
README.PARAMETER_PARSING_API).  Like I said, some functions could be updated 
to use this (with "limit" or "length" type params) to prevent 
overflow-related weirdness if huge numbers are passed.  It should be noted 
that such unexpected behavior has existed through 5.2, however, though the 
changes in 5.3 started to prevent it in a lot of cases (very inconsistent 
though; Bug #47854, etc.).


Thanks,
Matt

(Previous message continues below, for some explanation of final version.)

> After seeing how things work on Windows and [new for me] 32- and 64-bit 
> Linux, there's no longer all that magic stuff I had before. :-)  Now the 
> conversion method is basically just like what 5.2 has, and its behavior 
> that's been there for years (I assume, for the majority of users, at least 
> for positive numbers).  The difference is that on 32-bit, it now uses a 
> (zend_long64) cast, which should fix the inconsistency with the Mac that 
> Zoe reported in Bug #42868, which lead to the change in 5.3.  (It appears 
> that the (unsigned long) cast was limiting to ULONG_MAX, instead of 
> overflowing and preserving least significant bits like other platforms.)
>
> NOTE: The (zend_long64) part is exactly the same thing you did (to ensure 
> overflow?), for positive values, in April '07, zend_operators.c v1.269 for 
> "WIN64 support" (I don't think Win64 is any different than Win32 -- a 
> difference in compilers between VC6 and newer? Yes).
>
> This should handle the 64-bit range of values.  From what I've been able 
> to check, once a double's exponent is above 64 (or is it 63? whatever), 
> the conversion doesn't work (no different than PHP 5.2), without fmod... 
> Outside of -2^63 and 2^63 on 32-bit Linux/Windows, you get 0; on 64-bit 
> Linux (*like 5.2*), above 2^64 is 0 and below -2^63 is LONG_MIN.
>
> I also added a configure check and flag if a simple (long) cast is 
> sufficient to preserve LSB.  Don't know if I put it in the "right" place 
> (Zend.m4) or if it should be kept, but it's there.
>
> BTW, I used a function so it's easier to use than the current macro.  With 
> zend_always_inline, it should usually be the same...?  I would have used a 
> macro with ?: but that was used previously before removal from 
> zend_operators.c in Sep '06, v1.256: "use if() instead of ?: and avoid 
> possible optimization problems."