Re: [VOTE][RFC] Integer Semantics
| From: | Chris Wright | Date: | Tue, 16 Sep 2014 10:19:25 +0000 |
| Subject: | Re: [VOTE][RFC] Integer Semantics | ||
| References: | 1 2 3 4 5 6 | Groups: | php.internals |
| Request: | Send a blank email to internals+get-77242@lists.php.net to get a copy of this message | ||
On 16 September 2014 11:05, Dmitry Stogov <dmitry@zend.com> wrote:
> you already made silent break for N << 64 and N >> 64, but it may be
> explained as more consistent behaviour.
> I don't see a big difference with negative shifts.
>
> The real thing that I don't like - is a "boolean" result. Warning is not a
> big problem.
I'm inclined to agree with this. The warning makes sense as it's
almost certainly a userland bug, and if the expected old behaviour is
desired it can easily be modified in userland, but returning FALSE
doesn't make a huge amount of sense. This will almost certainly be
immediately cast to int(0) by the next operation - almost no-one is
going to actually check for a return value of FALSE - so it makes more
sense to just return 0 and avoid the implicit cast. This would also
produce possibly-unexpected results if a future operation was to
stringify the result, as FALSE would cast to the empty string.
I have voted in favour of the RFC as it stands as I believe that
overall the changes are positive, but ideally this particular case
would be addressed.
>
> Thanks. Dmitry.
>
>
>
> On Tue, Sep 16, 2014 at 1:23 PM, Andrea Faulds <ajf@ajf.me> wrote:
>
>>
>> On 16 Sep 2014, at 10:16, Dmitry Stogov <dmitry@zend.com> wrote:
>>
>> > Shifts by negative number may make sense. (N << -1) => (N >> 1)
>> > At least receiving "false" from shift is not very pleasant.
>>
>> The problem is that changing from the current behaviour (undefined in C,
>> but typically a shift by (PHP_INT_MAX - $bits)) to do what you’d expect
>> (shift in the opposite direction) would be a silent BC break. I think it’s
>> better to just stop it altogether and raise an E_WARNING, where it’s at
>> least obvious something went wrong, than change this behaviour silently.
>>
>> > In the patch you use SIZEOF_LONG. It probably should be changed to
>> > SIZEOF_ZEND_LONG.
>>
>> Will fix, thanks for spotting that.
>>
>> --
>> Andrea Faulds
>> http://ajf.me/
>>
>>
>>
>>
>>