-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Rewrite $a ?: $b to empty($a) ? $b : $a if $a is a variable #809
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
Conversation
zend_compile_shorthand_conditional(result, ast TSRMLS_CC); | ||
return; | ||
/* if variable is isset()able, rewrite $a ?: $b to empty($a) ? $b : $a */ | ||
if (zend_is_variable(cond_ast) && !zend_is_call(cond_ast)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that a behavior change? Before, if $a didn't exist, there were notice, but after, there's no notice anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A behaviour change, yes, but it doesn't break backwards-compatibility. I'm writing an RFC for this as we speak, just to clarify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it not break BC? You have no idea what people might do with that notice in their custom error handlers. If you remove it, that code won't be triggered and the app will behave differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it won't break BC for most applications, but it will break things if they relied on it throwing an E_NOTICE
. I'll update the RFC to note that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I didn't realize that. Within the context of RFC it makes sense. I'd support that for PHP 7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a bit inconsistent to me that $a ? $a : $b will throw a notice and $a ?: $b won't. We would need to make that change extremely clear in the documentation highlighting the fact that ?: is now the first operator to suppress these notices and that it is now fundamentally different from its cousin, the ? expr1 : expr2 operator.
👍 |
I think even this is acceptable, it should also be done in opcache.. |
@laruence are you saying I should rewrite the AST in opcache? Because the opcodes generated would just be cached as usual. |
@TazeTSchnitzel I misunderstood your point as a compiler optimization.. ingore my previous comment. |
@laruence I'd thought of just silencing errors, but I realised that causes a lot of problems. For example, it wouldn't just silence nonexistant variable/indes warnings, and I'd have to handle nested silencing (what if it calls a function)? However, implementing it as just |
Actually, there's a second BC concern here. Because it's rewritten to |
Well, I thought the whole point of ?: would be to use $a result immediately if it's not empty and not try to do anything else. Doing it twice definitely not a good option. |
@smalyshev Right, which is why I need to figure out a workaround to avoid doubly evaluating it if possible. |
@TazeTSchnitzel hmm, in such case, I am not sure, but maybe a new syntax is more clear? PHP is a interpreting language, it's better to always make opcodes exactly reflects what the PHP codes is(at least before optimization of course) |
Don't worry, this is just a current problem in the patch, I'm going to figure out a way to do it that avoids double evaluation. I guess I'll have to add a new type of isset/empty operation internally. |
I prefer that you implement it using isset than empty ...because the value "false" can be set and meaningfull. the main point is to have only one access to the variable to be tested, in order to have a x2 speedup! What about implementing a new opcode (via a new operator or a new function, whatever the syntax is) to achieve the functionnality in an "atomic" way ? two forms of this functionality have to be considered: where $condition could be only keywords (i.e. isset , !isset, eempty, !empty, is not null, is_empty , type==double) or an expression (i.e.<=$b ) the set form would replace the $a value by the new one. I repeat, the syntax has to be defined, but this functionnality wille be definitly needed when a real native implementation of multithreading will be implemented within php! |
@TazeTSchnitzel will this notation also work for objects? All examples I have seen so far contained only arrays...? |
@staabm Yes, see the test. Though I'm now focussing on making a separate |
Actually, I like the ?= idea.... |
I don't, it would be confusing for newbies, since it looks like an assignment operator (like I was thinking about: |
The patch for this RFC: https://wiki.php.net/rfc/isset_ternary