On 2023/02/19 09:45, Nikita Popov <nikita....@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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
