-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Strict comparison for switch cases #3297
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
if (!equality_op) { | ||
equality_op = (expr_node.op_type & (IS_VAR|IS_TMP_VAR)) ? ZEND_CASE : ZEND_IS_EQUAL; | ||
} | ||
opline = zend_emit_op(NULL, equality_op, &expr_node, &cond_node); |
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.
Won't you be freeing expr_node many many times here?
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.
Probably, like I said... it's quick and dirty and more for demonstration.
Curiously, my debug build isn't complaining about it.
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.
Oh, duh, because it's a CV in my tests, I'll bet a TMP_VAR will choke.
What about switch($someValue) {
case < 10: break;
case < 20: break;
case < 30: break;
case < 40: break;
default: break;
} |
from a reading POV I would prefer having the variable in the near of the conditon, e.g. switch {
case $someValue < 10: break;
case $someValue > 20: break;
case $someValue === 30: break;
case $someValue < 40: break;
default: break;
} |
@staabm you could already do that with: switch (true) {
case $someValue < 10: break;
case $someValue === 20: break;
case $someValue >= 30: break;
default: break;
} Just as I've seen this implemented in the user land occasionally. However from my understanding only a single reference to |
@staabm Uhh, I wouldn't do that. If you do want that, too confusing and too easily abusable. |
I still don't like the syntax suggestions here. :( I think this should rather be done as part of introducing |
I really like the idea of |
@fprochazka I like the idea, but does that include |
IMO
This way we're not changing language syntax at all, and it's easier in a future if it won't be accepted by the community to mute declare than remove syntax. |
In addition to those mentioned by @brzuchal, it would also be an opportunity to fix the following:
What else? :) |
I personally would be strict everywhere without any explicit declaration. Be non-strict is antipattern. |
@EdaCZ yeah, me to but you need to keep backward compatibility in major versions of PHP that's why introducing declare is the only way to change it per case/file. |
Dropping some syntax suggestion: like $foo = false;
switch ($foo, true) {
case 0: // should not enter
case null: // should not enter
case false: // should enter \o/
} |
@carusogabriel |
@Majkl578 @sgolemon I personally don't like the idea of universal or file-scoped strict comparison, unless it's for scalar values only. Objects are a special case. Consider the case where you want strict comparisons for scalars (no type juggling), where Maybe strict comparison shouldn't affect objects? So that == is still object-by-property (or __equals in #3339) and === is still object-same-reference. Otherwise the only option we have is to compare reference-to-reference with no way to compare objects for equality like we do today. |
Simple: This thing would cease to exist as it shares the same garbage behavior as type juggling and
As said above, strict comparisons would apply to in_array too (it's weak behavior would no longer be default). (But who uses non-strict in_array anyway?) |
Yup, so the default value for the third param in Strict comparison ( Objects: Same exact instance, same object reference. Those aren't the same at all. |
For discussion...
Jump table optimizations should be unimpacted since I believe they are already strict, but I haven't confirmed this yet.