[VOTE][RFC] ZPP Failure on Overflow

Laruence

11 years ago

Hey:

On Wed, Dec 3, 2014 at 5:44 AM, Andrea Faulds <ajf@ajf.me> wrote:

> Good evening once again,
>
> I’m putting the ZPP Failure on Overflow RFC to a vote. This RFC is an important prerequisite to the Big Integer Support RFC, as it ensures safety in implicit integer parameter conversions.
>
> The RFC requires a 2/3 majority, and voting starts today (2014-12-02) and ends in 10 days’ time next Friday (2014-12-12).
>
> Please read the RFC carefully. If you have any doubts or queries about the RFC, please don’t hesitate to ask me.
>
> The RFC and voting widget can be found here: https://wiki.php.net/rfc/zpp_fail_on_overflow
>
> Thanks!

I am not sure if I misunderstand your proposal..

I thought you proposal is only fail if double value is out of long range

but from your patch, ext/date/tests/date_sunset_variation9.phpt  is
fails with a double value 12.*** (which is obviously not out of long
range).

thanks

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

--
Xinchen Hui
@Laruence
http://www.laruence.com/

Andrew Faulds

11 years ago

> On 4 Dec 2014, at 05:51, Xinchen Hui <laruence@php.net> wrote:
>
>>
> I am not sure if I misunderstand your proposal..
>
> I thought you proposal is only fail if double value is out of long range
>
> but from your patch, ext/date/tests/date_sunset_variation9.phpt  is
> fails with a double value 12.*** (which is obviously not out of long
> range).

That one caught me out too! Look more carefully, I missed it the first time.

12.3456789000e10 has a sneaky “e10” at the end, so it’s more like 123456789000 ;)

--
Andrea Faulds
http://ajf.me/

Laruence

11 years ago

On Thu, Dec 4, 2014 at 1:54 PM, Andrea Faulds <ajf@ajf.me> wrote:

>
>> On 4 Dec 2014, at 05:51, Xinchen Hui <laruence@php.net> wrote:
>>
>>>
>> I am not sure if I misunderstand your proposal..
>>
>> I thought you proposal is only fail if double value is out of long range
>>
>> but from your patch, ext/date/tests/date_sunset_variation9.phpt  is
>> fails with a double value 12.*** (which is obviously not out of long
>> range).
>
> That one caught me out too! Look more carefully, I missed it the first time.
>
> 12.3456789000e10 has a sneaky “e10” at the end, so it’s more like 123456789000 ;)

Ah...... okey.... :)

thanks

>
> --
> Andrea Faulds
> http://ajf.me/
>
>
>
>

--
Xinchen Hui
@Laruence
http://www.laruence.com/

Andrew Faulds

11 years ago

Hi!

We’re now half-way through voting on this RFC, and only three people, two if I’m excluded, have voted. I realise it’s a rather technical and obscure matter, but I am a little worried by how few people have voted so far. Three votes is hardly enough to justify a change.

If you missed the original announcement, hopefully this reminder will reach you. Here it is, repeated:

> On 2 Dec 2014, at 21:44, Andrea Faulds <ajf@ajf.me> wrote:
>
> I’m putting the ZPP Failure on Overflow RFC to a vote. This RFC is an important prerequisite to the Big Integer Support RFC, as it ensures safety in implicit integer parameter conversions.
>
> The RFC requires a 2/3 majority, and voting starts today (2014-12-02) and ends in 10 days’ time next Friday (2014-12-12).
>
> Please read the RFC carefully. If you have any doubts or queries about the RFC, please don’t hesitate to ask me.
>
> The RFC and voting widget can be found here: https://wiki.php.net/rfc/zpp_fail_on_overflow

Thanks!

--
Andrea Faulds
http://ajf.me/

Joe Watkins

11 years ago

On Sun, 2014-12-07 at 22:03 +0000, Andrea Faulds wrote:

> Hi!
>
> We’re now half-way through voting on this RFC, and only three people, two if I’m excluded, have voted. I realise it’s a rather technical and obscure matter, but I am a little worried by how few people have voted so far. Three votes is hardly enough to justify a change.
>
> If you missed the original announcement, hopefully this reminder will reach you. Here it is, repeated:
>
> > On 2 Dec 2014, at 21:44, Andrea Faulds <ajf@ajf.me> wrote:
> >
> > I’m putting the ZPP Failure on Overflow RFC to a vote. This RFC is an important prerequisite to the Big Integer Support RFC, as it ensures safety in implicit integer parameter conversions.
> >
> > The RFC requires a 2/3 majority, and voting starts today (2014-12-02) and ends in 10 days’ time next Friday (2014-12-12).
> >
> > Please read the RFC carefully. If you have any doubts or queries about the RFC, please don’t hesitate to ask me.
> >
> > The RFC and voting widget can be found here: https://wiki.php.net/rfc/zpp_fail_on_overflow
>
> Thanks!
>
> --
> Andrea Faulds
> http://ajf.me/
>
>
>
>
>

Morning Andrea,

I think it's okay to assume that people are winding down for the
holiday, possibly some people are already on holiday.

Because of the time of year, you might want to leave the vote open
another week, or until the number of votes justifies the change.

Cheers
Joe

Andrew Faulds

11 years ago

> On 8 Dec 2014, at 06:24, Joe Watkins <pthreads@pthreads.org> wrote:
>
> Morning Andrea,
>
>    I think it's okay to assume that people are winding down for the
> holiday, possibly some people are already on holiday.
>
>    Because of the time of year, you might want to leave the vote open
> another week, or until the number of votes justifies the change.
>
> Cheers
> Joe

Morning Joe,

I agree, the timing probably has something to do with it. If it hasn't got many votes by Friday, I'll extend it.

Cheers.

--
Andrea Faulds
http://ajf.me/

Laruence

11 years ago

Hey:

On Mon, Dec 8, 2014 at 6:03 AM, Andrea Faulds <ajf@ajf.me> wrote:

>
> Hi!
>
> We’re now half-way through voting on this RFC, and only three people, two if I’m excluded, have voted. I realise it’s a rather technical and obscure matter, but I am a little worried by how few people have voted so far. Three votes is hardly enough to justify a change.

the current warning message is :" expects parameter 1 to be long,
double given in %s on line %d"

but maybe it should be something like: "expects parameter 1 to be
long, double(which is beyond long range) given in %s on line %d"?

since it's not the fault of double self, right?

thanks

>
> If you missed the original announcement, hopefully this reminder will reach you. Here it is, repeated:
>
>> On 2 Dec 2014, at 21:44, Andrea Faulds <ajf@ajf.me> wrote:
>>
>> I’m putting the ZPP Failure on Overflow RFC to a vote. This RFC is an important prerequisite to the Big Integer Support RFC, as it ensures safety in implicit integer parameter conversions.
>>
>> The RFC requires a 2/3 majority, and voting starts today (2014-12-02) and ends in 10 days’ time next Friday (2014-12-12).
>>
>> Please read the RFC carefully. If you have any doubts or queries about the RFC, please don’t hesitate to ask me.
>>
>> The RFC and voting widget can be found here: https://wiki.php.net/rfc/zpp_fail_on_overflow
>
> Thanks!
>
> --
> Andrea Faulds
> http://ajf.me/
>
>
>
>
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>

--
Xinchen Hui
@Laruence
http://www.laruence.com/

Andrew Faulds

11 years ago

> On 8 Dec 2014, at 08:22, Xinchen Hui <laruence@php.net> wrote:

> the current warning message is :" expects parameter 1 to be long,
> double given in %s on line %d"
>
> but maybe it should be something like: "expects parameter 1 to be
> long, double(which is beyond long range) given in %s on line %d"?
>
> since it's not the fault of double self, right?

Hi Xinchen,

I had thought of doing that, although for all other failures we also don't give  special error messages. I don't have the time to go through and update every single test again, though. I initially did have an error message that showed the range that the float needs to have, but this was a problem because it means the error message might vary by platform (pain to test)

--
Andrea Faulds
http://ajf.me/

Laruence

11 years ago

Hey:

On Mon, Dec 8, 2014 at 7:15 PM, Andrea Faulds <ajf@ajf.me> wrote:

>
>> On 8 Dec 2014, at 08:22, Xinchen Hui <laruence@php.net> wrote:
>
>> the current warning message is :" expects parameter 1 to be long,
>> double given in %s on line %d"
>>
>> but maybe it should be something like: "expects parameter 1 to be
>> long, double(which is beyond long range) given in %s on line %d"?
>>
>> since it's not the fault of double self, right?
>
> Hi Xinchen,
>
> I had thought of doing that, although for all other failures we also don't give  special error messages. I don't have the time to go through and update every single test again, though. I initially did have an error message that showed the range that the float needs to have, but this was a problem because it means the error message might vary by platform (pain to test)

that will confuse people, why 12.2 is okey (it's also a double value).

anyway, I am not a native english speaker, not sure what is the most
proper warning message.

but I suggest change it to "expects parameter 1 to be long,
double(which is beyond long range) given in %s on line %d"

what do you think?

thanks

>
> --
> Andrea Faulds
> http://ajf.me/

--
Xinchen Hui
@Laruence
http://www.laruence.com/

Andrew Faulds

11 years ago

Hi!

> On 9 Dec 2014, at 03:39, Xinchen Hui <laruence@php.net> wrote:
>
> Hey:

> that will confuse people, why 12.2 is okey (it's also a double value).
>
> anyway, I am not a native english speaker, not sure what is the most
> proper warning message.
>
> but I suggest change it to "expects parameter 1 to be long,
> double(which is beyond long range) given in %s on line %d"
>
> what do you think?

I realise the behaviour is somewhat confusing, but we also do this for other parameter parsing failures and I don't have the time to go through and change all the tests again, unfortunately. I'd also need to change how error messages are produced for fast parameter parsing.

Thanks.

--
Andrea Faulds
http://ajf.me/

Rowan Collins

11 years ago

Xinchen Hui wrote on 09/12/2014 03:39:

> but I suggest change it to "expects parameter 1 to be long,
> double(which is beyond long range) given in %s on line %d"

Note that in master, the messages have been changed to (correctly) not
mention the C types, only the PHP ones, so it would be more like
"expects parameter 1 to be integer, float (which is beyond integer
range) given".

Andrew Faulds

11 years ago

> On 9 Dec 2014, at 13:23, Rowan Collins <rowan.collins@gmail.com> wrote:
>
> Xinchen Hui wrote on 09/12/2014 03:39:
>> but I suggest change it to "expects parameter 1 to be long,
>> double(which is beyond long range) given in %s on line %d"
>
> Note that in master, the messages have been changed to (correctly) not mention the C types, only the PHP ones, so it would be more like "expects parameter 1 to be integer, float (which is beyond integer range) given".

They... have? When did this happen? It's something I was very much in favour of and wanted to do myself, but I was unaware this had recently changed. Does that mean I'll have to redo the patch for this RFC? :(

--
Andrea Faulds
http://ajf.me/

Rowan Collins

11 years ago

Andrea Faulds wrote on 09/12/2014 14:10:

>> On 9 Dec 2014, at 13:23, Rowan Collins <rowan.collins@gmail.com> wrote:
>>
>> Xinchen Hui wrote on 09/12/2014 03:39:
>>> but I suggest change it to "expects parameter 1 to be long,
>>> double(which is beyond long range) given in %s on line %d"
>> Note that in master, the messages have been changed to (correctly) not mention the C types, only the PHP ones, so it would be more like "expects parameter 1 to be integer, float (which is beyond integer range) given".
> They... have? When did this happen? It's something I was very much in favour of and wanted to do myself, but I was unaware this had recently changed. Does that mean I'll have to redo the patch for this RFC? :(
>

Oh; maybe not. I'm confused now.

There's a commit making the change, I think in amongst the "64-bit"
work:
https://github.com/php/php-src/commit/63ea29a2067ce06e20dda861480d91624389f0f3

But then the current master seems to be back using the old wording
(random example):
https://github.com/php/php-src/blob/master/ext/curl/tests/curl_setopt_error.phpt
(Also, lxr.php.net doesn't show the PHPT files at all?
http://lxr.php.net/xref/PHP_TRUNK/ext/curl/tests/)

I actually prepared a PR a while ago to change everything, but master
was a bit of a moving target at the time, so it probably needs a bit of
work to rebase: https://github.com/php/php-src/pull/769 I didn't bother
because I thought someone had beaten me to it, but maybe I should
revisit it after all?

--
Rowan Collins
[IMSoP]

Andrew Faulds

11 years ago

> On 9 Dec 2014, at 15:27, Rowan Collins <rowan.collins@gmail.com> wrote:
>
> Andrea Faulds wrote on 09/12/2014 14:10:
>>> On 9 Dec 2014, at 13:23, Rowan Collins <rowan.collins@gmail.com> wrote:
>>>
>>> Note that in master, the messages have been changed to (correctly) not mention the C types, only the PHP ones, so it would be more like "expects parameter 1 to be integer, float (which is beyond integer range) given".
>> They... have? When did this happen? It's something I was very much in favour of and wanted to do myself, but I was unaware this had recently changed. Does that mean I'll have to redo the patch for this RFC? :(
>>
>
> Oh; maybe not. I'm confused now.
>
> There's a commit making the change, I think in amongst the "64-bit" work: https://github.com/php/php-src/commit/63ea29a2067ce06e20dda861480d91624389f0f3
>
> But then the current master seems to be back using the old wording (random example): https://github.com/php/php-src/blob/master/ext/curl/tests/curl_setopt_error.phpt (Also, lxr.php.net doesn't show the PHPT files at all? http://lxr.php.net/xref/PHP_TRUNK/ext/curl/tests/)
>
> I actually prepared a PR a while ago to change everything, but master was a bit of a moving target at the time, so it probably needs a bit of work to rebase: https://github.com/php/php-src/pull/769 I didn't bother because I thought someone had beaten me to it, but maybe I should revisit it after all?

Hi!

It looks like those changes aren’t in master. Once this RFC passes, I’d very much like to see a new attempt to make the names consistent, it’s something that’s bothered me. If this was to be revisited, I think the names for IS_LONG and IS_BOOL should be “int” and “bool” respectively, not “integer” and “boolean”, those feel more like proper type names. Also, it might be worth making is_long an alias of is_int, rather than the reverse which is what we have now.

Thanks!

--
Andrea Faulds
http://ajf.me/

Rowan Collins

11 years ago

Andrea Faulds wrote on 10/12/2014 16:04:

>> On 9 Dec 2014, at 15:27, Rowan Collins <rowan.collins@gmail.com> wrote:
>>
>> Andrea Faulds wrote on 09/12/2014 14:10:
>>>> On 9 Dec 2014, at 13:23, Rowan Collins <rowan.collins@gmail.com> wrote:
>>>>
>>>> Note that in master, the messages have been changed to (correctly) not mention the C types, only the PHP ones, so it would be more like "expects parameter 1 to be integer, float (which is beyond integer range) given".
>>> They... have? When did this happen? It's something I was very much in favour of and wanted to do myself, but I was unaware this had recently changed. Does that mean I'll have to redo the patch for this RFC? :(
>>>
>> Oh; maybe not. I'm confused now.
>>
>> There's a commit making the change, I think in amongst the "64-bit" work: https://github.com/php/php-src/commit/63ea29a2067ce06e20dda861480d91624389f0f3
>>
>> But then the current master seems to be back using the old wording (random example): https://github.com/php/php-src/blob/master/ext/curl/tests/curl_setopt_error.phpt (Also, lxr.php.net doesn't show the PHPT files at all? http://lxr.php.net/xref/PHP_TRUNK/ext/curl/tests/)
>>
>> I actually prepared a PR a while ago to change everything, but master was a bit of a moving target at the time, so it probably needs a bit of work to rebase: https://github.com/php/php-src/pull/769 I didn't bother because I thought someone had beaten me to it, but maybe I should revisit it after all?
> Hi!
>
> It looks like those changes aren’t in master. Once this RFC passes, I’d very much like to see a new attempt to make the names consistent, it’s something that’s bothered me. If this was to be revisited, I think the names for IS_LONG and IS_BOOL should be “int” and “bool” respectively, not “integer” and “boolean”, those feel more like proper type names. Also, it might be worth making is_long an alias of is_int, rather than the reverse which is what we have now.
>
> Thanks!

Regarding the naming, "integer" and "boolean" are used throughout the
manual, and are what is returned by gettype(). (Unfortunately, it
returns "double" for floats, but it's probably not worth the BC pain of
changing that.) On the other hand, var_dump() uses "int" and "bool", and
is_int() and is_bool() are the documented function names, so there is
some precedent either way I suppose.

My personal opinion is that the abbrevs make the messages less
user-friendly (remember, these messages are for ordinary PHP developers,
not C gurus), and don't really have any advantage over the full words,
but I'm willing to be persuaded otherwise.

Regards,

--
Rowan Collins
[IMSoP]

Pascal MARTIN

11 years ago

On 02/12/2014 22:44, Andrea Faulds wrote:

> I’m putting the ZPP Failure on Overflow RFC to a vote. This RFC is an important prerequisite to the Big Integer Support RFC, as it ensures safety in implicit integer parameter conversions.
>
> The RFC requires a 2/3 majority, and voting starts today (2014-12-02) and ends in 10 days’ time next Friday (2014-12-12).

Hi,

Opinions from the community might not be the most interesting ones for
this kind of RFC, but the few members of AFUP who took part in a (short)
discussion about it would be +1.

--
Pascal MARTIN, AFUP - French UG
http://php-internals.afup.org/

Andrew Faulds

11 years ago

> On 2 Dec 2014, at 21:44, Andrea Faulds <ajf@ajf.me> wrote:
>
> Good evening once again,
>
> I’m putting the ZPP Failure on Overflow RFC to a vote. This RFC is an important prerequisite to the Big Integer Support RFC, as it ensures safety in implicit integer parameter conversions.
>
> The RFC requires a 2/3 majority, and voting starts today (2014-12-02) and ends in 10 days’ time next Friday (2014-12-12).
>
> Please read the RFC carefully. If you have any doubts or queries about the RFC, please don’t hesitate to ask me.
>
> The RFC and voting widget can be found here: https://wiki.php.net/rfc/zpp_fail_on_overflow
>
> Thanks!

Good evening,

Voting on this RFC has ended. By sixteen votes to none against, the RFC is accepted. I shall hopefully merge the patch into master sometime soon.

This has been the second prerequisite RFC for the Big Integer Support RFC, with the first having been the Integer Support RFC. There may still be one more of these in order to deal with array key overflow handling, but I am yet to write such an RFC or an implementation.

Thanks to everyone for participating in the vote and discussion!

--
Andrea Faulds
http://ajf.me/