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)
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.
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...]).
Btw. I see your point about the arbitary number. But I also feel that it
would be better to keep it as long as it would keep the same behavior when
passed to external libs.
Cheers
Jakub