-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
Implement GH-18261: Allow cast to be used in constant expressions #18264
base: master
Are you sure you want to change the base?
Conversation
const T4 = (object) ["a" => 1]; | ||
const T5 = (float) 5; | ||
const T6 = (array) ""; | ||
const T7 = (array) var_dump(...); |
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.
Test with warning like https://3v4l.org/6ugOa should be added.
IDK if exception is possible, if so, it should be tested as well.
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.
Exceptions are at least possible with (string) $nonStringable
, there might be others.
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.
I added the warning thing, but exceptions are indeed already possible.
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.
I think the point is that the IS_STRING
branch should set ret
to FAILURE
if the cast fails. I realized that we don't handle this incorrectly for destructors too. Maybe we should add an if (EG(exception)) { ret = FAILURE; }
right before the return. That said, this seems to not cause any issues because neither constructors nor destructors are called from zend_ast_evaluate_inner()
when EG(exception)
is set. So maybe it's ok, zend_ast_evaluate_inner()
will just do too much work when an exception occurs early.
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.
LGTM otherwise.
break; | ||
case IS_ARRAY: | ||
/* Adapted from VM */ | ||
if (Z_TYPE(op1) != IS_OBJECT || Z_OBJCE(op1) == zend_ce_closure) { |
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.
This looks like almost a direct copy. Can we move it to an inlined function?
} | ||
break; | ||
case IS_OBJECT: | ||
/* Adapted from VM */ |
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.
Same here.
No description provided.