[VOTE][RFC] ZPP Failure on Overflow

11 years ago by Andrea Faulds — view source — reply

unread

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!

Andrea Faulds
http://ajf.me/

11 years ago by Xinchen Hui — view source — reply

unread

Hey:

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/

--

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

11 years ago by Andrea Faulds — view source — reply

unread

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/

11 years ago by Xinchen Hui — view source — reply

unread

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/

11 years ago by Andrea Faulds — view source — reply

unread

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:

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/

11 years ago by Joe Watkins — view source — reply

unread

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:

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

11 years ago by Andrea Faulds — view source — reply

unread

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/

11 years ago by Xinchen Hui — view source — reply

unread

Hey:

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:

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/

--

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

11 years ago by Andrea Faulds — view source — reply

unread

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/

11 years ago by Xinchen Hui — view source — reply

unread

Hey:

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/

11 years ago by Andrea Faulds — view source — reply

unread

Hi!

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/

11 years ago by Rowan Collins — view source — reply

unread

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".

11 years ago by Andrea Faulds — view source — reply

unread

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/

11 years ago by Rowan Collins — view source — reply

unread

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

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]

11 years ago by Andrea Faulds — view source — reply

unread

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

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/

11 years ago by Pascal Martin — view source — reply

unread

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/

11 years ago by Andrea Faulds — view source — reply

unread

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/

11 years ago by Andrea Faulds — view source — reply

unread

Hi,

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

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.

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.

I just want it to be consistent, really. With the old Scalar Type Hinting RFC, I ended up reserving the type names “int”, “float”, “string” and “bool”. The problem is that we also call ints “integers” and bools “booleans”, and I’d prefer for us to choose one and stick with it. “int” and “bool” are shorter, and I’m pretty sure PHP developers know what they are (var_dump uses those, as you mentioned). They’re widely used in other languages, too. The manual does use “integer” and “boolean”, but all the function signatures use “int” and “bool”.

I suppose it doesn’t matter, ultimately, if we use “int” and “integer” or “bool” and “boolean” interchangeably, except perhaps in future syntax to avoid adding too many reserved words. What we need to eliminate is “double” and “long”.

Andrea Faulds
http://ajf.me/