Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:119576
Date: Sun, 19 Feb 2023 11:32:37 +0100
To: Nikita Popov <nikita.ppv@gmail.com>
Cc: internals@lists.php.net
Message-ID: <Y/H6xZHOcg2Fpyqv@swift.blarg.de>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <478d639f-9f09-4747-a21a-eb4bceb5c053@app.fastmail.com>
Subject: Re: [PHP-DEV] What's the purpose of zend_result?
From: max+php@blarg.de (Max Kellermann)

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.

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