-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Refactored some ZPP calls in OCI8. #5714
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
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.
Thanks for your work! All in all, the PR looks good to me as-is. I only had a few small nits, but I'm not 100% sure it's worth to address them right now. So let's now wait for other reviews.
if (ZEND_NUM_ARGS() == 2) { | ||
data_len = MIN((zend_long) data_len, write_len); | ||
} | ||
if (getThis() && ZEND_NUM_ARGS() == 2) { |
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.
These consecutive checks everywhere could be optimized a tiny little bit by either:
- using
else if
clauses - extracting
getThis()
in an outer condition so that this function is not called multiple times
However, all the ZEND_NUM_ARGS() == ...
checks indicate that the default value handling is not right - which is something we want to fix later. That's why I'm also ok with leaving these conditions as you have them currently.
ping @cjbj |
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.
If the tests pass, LGTM
Do we have |
@carusogabriel Nope. :( I believe Jens run the tests locally. @jensdenies, so the tests pass, right? |
Traditionally only gcov ran Oracle DB. But I guess http://gcov.php.net/ is obsolete? |
@cjbj Yes, gcov is no longer used. To test oci, we'd need to have it on either Travis, Azure Pipelines or AppVeyor. It does look like installing it on travis is possible: https://github.com/Vincit/travis-oracledb-xe No idea how compatible that is with licensing. |
I've opened php/php-tasks#5 to track testing oci8 on CI, though from a cursory look, it doesn't look like a simple thing right now. |
Thanks to the comments of @kocsismate on #5701 i refactored some ZPP calls to the "zend_parse_method_parameters" variant, which ensures ZPP is always called on method/function invocation.
I also moved the "old_oci_close_semantics" check beneath the ZPP check.