Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:119577
Message-ID: <ea030a1f-6407-75b6-a079-eb90d8f0f8d5@gmail.com>
Date: Sun, 19 Feb 2023 11:56:20 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Content-Language: en-US
To: Max Kellermann <max+php@blarg.de>, Nikita Popov <nikita.ppv@gmail.com>
Cc: internals@lists.php.net
References: <Y/HQbAJqN+XBtlP0@swift.blarg.de>
 <a614d204-ebc8-4640-9e99-d3e66810a8f2@app.fastmail.com>
 <Y/HcAbFWkKgSozNU@swift.blarg.de>
 <478d639f-9f09-4747-a21a-eb4bceb5c053@app.fastmail.com>
 <Y/H6xZHOcg2Fpyqv@swift.blarg.de>
In-Reply-To: <Y/H6xZHOcg2Fpyqv@swift.blarg.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] What's the purpose of zend_result?
From: dossche.niels@gmail.com (Niels Dossche)

On 2/19/23 11:32, Max Kellermann wrote:
> On 2023/02/19 09:45, Nikita Popov <nikita.ppv@gmail.com> wrote:
>> I expect that there are two main reasons for that:
>>  - There are probably some places that return a (non-negative) value or FAILURE.
>>  - There are probably some places that check for success/failure using >= 0 and < 0. Another POSIX-ism.
>>
>> I don't think we endorse such usage, but it likely exists.
> 
> Oh.  That's .... bad.
> 
> (Again, with C++'s strictly-typed enums, this would be easy to find:
> if it compiles, it's correct.)
> 
>> Let me turn the question around: Is there some reason to change the value of FAILURE at this point?
> 
> Right now, I'm trying to understand this choice of number and the
> obscure code comment NOT explaining it.  A comprehensive understanding
> of a design is an important first step for any other decision.
> 
> And indeed there would be a good practical reason to change this.  For
> example, on x86, the check for -1 compiles to:
> 
>  83 f8 ff     cmp    $0xffffffff,%eax
> 
> While the check for false (or 0) compiles to:
> 
>  85 c0        test   %eax,%eax
> 
> On Aarch64, check for -1 is:
> 
>  3100041f     cmn    w0, #0x1
>  54000060     b.eq   1c <Foo>
> 
> While false/0 compile to just one conditional branch:
> 
>  35000060     cbnz   w0, 3c <Foo>
> 
> For such a basic definition used everywhere, this does make a
> difference

Just chiming in real quickly here for two small notes...

We could work around this issue by checking if (function() != SUCCESS)
instead of if (function() == FAILURE). Not sure if it gives any measurable
improvement though.

It's also worth noting that there's a couple of places where there's
just a check for if (function()) { failure code }. i.e. there is no
check for == FAILURE, it's just "implied". So if there would be changes
to their values, this needs to be taken into account or fixed. Although
there might be 3rd party extensions doing this kind of code as well.

> 
> It was rather clumsy to choose -1 for FAILURE because it leads to
> less-than-optimal machine code.  Larger machine code means slower
> execution.  For one central basic definition as zend_result that gets
> used everywhere, it can make a measurable difference.
> 
> (Note that using "bool" would still be faster than using an enum with
> 0 and 1, but everything's better than -1.)
> 
>>> If I understand the guideline correctly, then those examples (and
>>> countless others) are defective and should be fixed, correct?
>>
>> At least in principle, yes. Of course, actually doing a change may not always be worthwhile, especially as this might result in a silent behavior change for extensions (as putting the return value into an "if" would invert behavior now).
> 
> (... and yet again, C++ would have saved us from this pain by not
> allowing implicit bool casts - sorry for annoying you with my C++
> fanboyism, but any pain that stems from choosing C is self-inflicted.)
> 
> The zend_fibers thing is a fairly recent addition, yet nobody spotted
> this API mistake befor merging it.  I've submitted a PR to fix it:
> https://github.com/php/php-src/pull/10622
> 
> Surprisingly, CODING_STANDARDS.md doesn't mention this API convention.
> Maybe it should?
> 
> Max
>