Skip to content

Conversation

laruence
Copy link
Member

@laruence laruence commented Feb 2, 2015

  1. Drop ZEND_REGISTER/FETCH_RESOURCE
  2. Avoiding double checking of passed_id's type
  3. Resource types are passed by registers now.

}

ZEND_API void *zend_fetch_resource(zval *passed_id, int default_id, const char *resource_type_name, int *found_resource_type, int num_resource_types, ...)
ZEND_API void *zend_fetch_resource2(zend_resource *res, const char *resource_type_name, int *found_type, int resource_type1, int resource_type2)
Copy link
Member

Choose a reason for hiding this comment

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

We may remove "found_type" argument and use res->type in caller context instead.
We also may use res->ptr instead of returned value.
In this case we may change this function into zend_verify_resource().

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. make sense....
  2. we need to return a flag to tell if it is expected type

@smalyshev smalyshev added the RFC label Feb 2, 2015
@php-pulls php-pulls merged commit 6c7d3ce into php:master Feb 4, 2015
@@ -2765,7 +2765,9 @@ PHP_FUNCTION(curl_setopt)
return;
}

ZEND_FETCH_RESOURCE(ch, php_curl *, zid, -1, le_curl_name, le_curl);
if ((ch = (php_curl*)zend_fetch_resource(Z_RES_P(zid), le_curl_name, le_curl)) == NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how three lines of code instead of one is an improvement.

Copy link
Member Author

Choose a reason for hiding this comment

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

the improvement is that we use zend_resource directly as parameters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants