Hi Jakub,
On Sat, January 11, 2014 19:31, Jakub Zelenka wrote:
> On Sat, Jan 11, 2014 at 4:53 PM, Anatol Belski <[email protected]> wrote:
>
>
>> Hi Jakub,
>>
>>
>> On Sat, January 11, 2014 17:08, Jakub Zelenka wrote:
>>
>>> On Sat, Jan 11, 2014 at 3:29 PM, Anatol Belski <[email protected]> wrote:
>>>
>>>
>>>
>>>>
>>>> yep, that's exactly how it performs till now in the mainstream,
>>>> just replace ZEND_INT_MAX with LONG_MAX. Adding a warning to every
>>>> such case would cause a warning flood in many PHP apps, guaranteed
>>>> :) Whereby it
>>>> might be ok for debug mode maybe, I wouldn't do it as I can't
>>>> remember any WTFs about the behavior.
>>>>
>>>>
>>>>
>>> I meant adding warning only for overflow cases. Values in existing
>>> apps can't be bigger than LONG_MAX so I don't see how it could cause
>>> warning flood in the current PHP apps. I don't even think that users
>>> will be often using such a big values after the 64bit changes.
>>>
>>> The thing is that if I use value bigger than LONG_MAX after 64bit
>>> changes and I pass it to the function defined in extension that does
>>> not support it (use "l" in zpp), then I rather see warning than
>>> unexpected rounding
>> to
>>> LONG_MAX... There is no way how to find out (except looking to the
>>> ext sources) that the big values are not supported. If I get warning,
>>> I can
>>> fix it in the code straight away...
>>>
>> But isn't it the exact situation now on ILP32 vs LP64 systems? Like 32
>> vs 64 bit Linux? I haven't suggested 'l' to handle the param the old
>> way, just it to be an alias for 'i'. In the new code that would be as
>> dynamic as php_int_t is, however 'l' as alias would retain zpp
>> compatibility to the older PHP.
>>
>> For instance how much sense had such a centralized warding for the
>> snippet is_int(PHP_INT_MAX+1) ? There is a lot of functions accepting an
>> arbitrary numeric value and not depending on any library, why they
>> should throw a warning by default? Why should we throw a warning about
>> 32 bit in the 64
>> bit build? Furtehrmore, LONG_MAX is 64 bit on LP64 systems, that would
>> be the same as PHP_INT_MAX.
>>
>>
> This is of course just about LLP64 when the situation has changed. I
> think that we are talking about slightly different things. Sorry my first
> example was a bit incorrect (ZEND_INT_MAX = _I64_MAX which is bigger than
> LONG_MAX
> on LLP64)
well, there's no another chance to understand each other than using some
verbal devices :)
> What I was suggesting is keeping BC for code like this:
> ...
> long n; if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "l", &n) ==
> FAILURE) {
> return; } ...
> The warning would be thrown if it's bigger than LONG_MAX or smaller than
> LONG_MIN.
>
> If I understood correctly you are suggesting using php_int_t for n
> (defined
> in compat header as long if not defined in php.h)? The problem is that the
> type would be dependent on PHP versions and when you need to be sure
> that it's a long (which might be the case when you pass it to the another
> lib function), then you need to check PHP_API_VERSION and in case it's not
> long, you have to do range check.
If a library expects long, in the new code that's the issue on 32 bit
windows only. So yes, probably the way you describe is plausible, check
PHP_WIN32 and PHP_API_VERSION. Honestly, right at the place where I sit, I
can't remember any library working with long (well, timeval struct and so
on, not really libs). There are int, size_t, int64_t, ... so while the
case you describe is of course possible, it's rather an exception. Usually
a simple runtime range check will be good enough, if needed at all.
>
> In general, I think that would be better to keep BC for "l" and use it
> just for long values (the "l" actually stands for long... :) ). That could
> be achieved adding range checks with warning if the value is bigger than
> LONG_MAX or smaller than LONG_MIN in zend_parse_arg_impl (it would
> probably require small changes in zend_parse_arg [expected_value...]).
>
Just to remind, your very first reply was to the mail about the zpp
compatibility, namely so then one wouldn't have to touch zpp calls for
migration, only the variables. That's why i've suggested to alias the new
formats with old. If we let 'l' to do the old thing, then zpp will have to
be touched at many places to be replaced with 'i' with all the
corresponding consequences. So retaining the functional compatibliity with
'l', an easier migration way is not possible. Also I'm asking myself, how
essential it really is, in the light of how many libs will profit from one
or another way.
Regards
anatol