Skip to content

refactor: change zend_is_true to return bool #14301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 24, 2024

Conversation

morrisonlevi
Copy link
Contributor

@morrisonlevi morrisonlevi commented May 22, 2024

Previously this returned int. Many function actually take advantage of the fact this returns exactly 0 or 1. For instance, main/streams/xp_socket.c does:

sockopts |= STREAM_SOCKOP_IPV6_V6ONLY_ENABLED * zend_is_true(tmpzval);

And Zend/zend_compile.c does:

child = &ast->child[2 - zend_is_true(zend_ast_get_zval(ast->child[0]))];

I changed a few places trivially from int to bool, but there are still many places such as the object handlers which return int that should eventually be bool.

I'm not quite sure yet what to do with ext/opcache/jit/zend_jit_ir.c:

ref = ir_CALL_1(IR_BOOL, ir_CONST_FC_FUNC(zend_is_true), jit_ZVAL_ADDR(jit, op1_addr));

What I understand from IR_BOOL and such so far, is that this is probably correct for bool returns already, but it's worth mentioning for people more expert than I am to comment on.

Previously this returned `int`. Many function actually take advantage
of the fact this returns exactly 0 or 1. For instance,
`main/streams/xp_socket.c` does:

    sockopts |= STREAM_SOCKOP_IPV6_V6ONLY_ENABLED * zend_is_true(tmpzval);

And `Zend/zend_compile.c` does:

    child = &ast->child[2 - zend_is_true(zend_ast_get_zval(ast->child[0]))];

I changed a few places trivially from `int` to `bool`, but there are
still many places such as the object handlers which return `int` that
should eventually be `bool`.
@morrisonlevi morrisonlevi force-pushed the zend_is_true_returns_bool branch from 335b74c to 64281bd Compare May 22, 2024 21:16
@morrisonlevi
Copy link
Contributor Author

morrisonlevi commented May 23, 2024

The failed Travis tests occur on master as well. Everything looks good to me, so unless someone has a critique, I'd like to target this for master.

@morrisonlevi morrisonlevi marked this pull request as ready for review May 23, 2024 00:01
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't care about this kind of changes.
A quick code-review didn't show any problems.

@nielsdos
Copy link
Member

Ok for DOM and XSL

@morrisonlevi morrisonlevi merged commit c461b60 into php:master May 24, 2024
10 checks passed
@morrisonlevi morrisonlevi deleted the zend_is_true_returns_bool branch May 24, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants