Skip to content

Commit

Permalink
Replace getThis() by EX(This), when additional check is not necessary.
Browse files Browse the repository at this point in the history
  • Loading branch information
@dstogov
dstogov committed Nov 13, 2018
1 parent 6bb94ea commit c6ad0b9
Show file tree
Hide file tree
Showing 44 changed files with 569 additions and 753 deletions.
9 changes: 4 additions & 5 deletions Zend/zend_closures.c
View file
Expand Up @@ -49,7 +49,7 @@ ZEND_METHOD(Closure, __invoke) /* {{{ */
zend_function *func = EX(func);
zval *arguments = ZEND_CALL_ARG(execute_data, 1);

if (call_user_function(CG(function_table), NULL, getThis(), return_value, ZEND_NUM_ARGS(), arguments) == FAILURE) {
if (call_user_function(CG(function_table), NULL, &EX(This), return_value, ZEND_NUM_ARGS(), arguments) == FAILURE) {
RETVAL_FALSE;
}

Expand Down Expand Up @@ -112,7 +112,7 @@ static zend_bool zend_valid_closure_binding(
Call closure, binding to a given object with its class as the scope */
ZEND_METHOD(Closure, call)
{
zval *zclosure, *newthis, closure_result;
zval *newthis, closure_result;
zend_closure *closure;
zend_fcall_info fci;
zend_fcall_info_cache fci_cache;
Expand All @@ -126,8 +126,7 @@ ZEND_METHOD(Closure, call)
return;
}

zclosure = getThis();
closure = (zend_closure *) Z_OBJ_P(zclosure);
closure = (zend_closure *) Z_OBJ(EX(This));

newobj = Z_OBJ_P(newthis);

Expand Down Expand Up @@ -166,7 +165,7 @@ ZEND_METHOD(Closure, call)
fci_cache.object = fci.object = newobj;

fci.size = sizeof(fci);
ZVAL_COPY_VALUE(&fci.function_name, zclosure);
ZVAL_OBJ(&fci.function_name, &closure->std);
fci.retval = &closure_result;
fci.no_separation = 1;

Expand Down
28 changes: 14 additions & 14 deletions Zend/zend_exceptions.c
View file
Expand Up @@ -270,7 +270,7 @@ ZEND_METHOD(exception, __construct)
zend_class_entry *base_ce;
int argc = ZEND_NUM_ARGS();

object = getThis();
object = &EX(This);
base_ce = i_get_exception_base(object);

if (zend_parse_parameters_ex(ZEND_PARSE_PARAMS_QUIET, argc, "|SlO!", &message, &code, &previous, zend_ce_throwable) == FAILURE) {
Expand Down Expand Up @@ -314,7 +314,7 @@ ZEND_METHOD(exception, __construct)
ZEND_METHOD(exception, __wakeup)
{
zval value, *pvalue;
zval *object = getThis();
zval *object = &EX(This);
CHECK_EXC_TYPE(ZEND_STR_MESSAGE, IS_STRING);
CHECK_EXC_TYPE(ZEND_STR_STRING, IS_STRING);
CHECK_EXC_TYPE(ZEND_STR_CODE, IS_LONG);
Expand Down Expand Up @@ -353,7 +353,7 @@ ZEND_METHOD(error_exception, __construct)
return;
}

object = getThis();
object = &EX(This);

if (message) {
ZVAL_STR_COPY(&tmp, message);
Expand Down Expand Up @@ -404,7 +404,7 @@ ZEND_METHOD(exception, getFile)

DEFAULT_0_PARAMS;

ZVAL_COPY(return_value, GET_PROPERTY(getThis(), ZEND_STR_FILE));
ZVAL_COPY(return_value, GET_PROPERTY(&EX(This), ZEND_STR_FILE));
}
/* }}} */

Expand All @@ -416,7 +416,7 @@ ZEND_METHOD(exception, getLine)

DEFAULT_0_PARAMS;

ZVAL_COPY(return_value, GET_PROPERTY(getThis(), ZEND_STR_LINE));
ZVAL_COPY(return_value, GET_PROPERTY(&EX(This), ZEND_STR_LINE));
}
/* }}} */

Expand All @@ -428,7 +428,7 @@ ZEND_METHOD(exception, getMessage)

DEFAULT_0_PARAMS;

ZVAL_COPY(return_value, GET_PROPERTY(getThis(), ZEND_STR_MESSAGE));
ZVAL_COPY(return_value, GET_PROPERTY(&EX(This), ZEND_STR_MESSAGE));
}
/* }}} */

Expand All @@ -440,7 +440,7 @@ ZEND_METHOD(exception, getCode)

DEFAULT_0_PARAMS;

ZVAL_COPY(return_value, GET_PROPERTY(getThis(), ZEND_STR_CODE));
ZVAL_COPY(return_value, GET_PROPERTY(&EX(This), ZEND_STR_CODE));
}
/* }}} */

Expand All @@ -452,7 +452,7 @@ ZEND_METHOD(exception, getTrace)

DEFAULT_0_PARAMS;

ZVAL_COPY(return_value, GET_PROPERTY(getThis(), ZEND_STR_TRACE));
ZVAL_COPY(return_value, GET_PROPERTY(&EX(This), ZEND_STR_TRACE));
}
/* }}} */

Expand All @@ -464,7 +464,7 @@ ZEND_METHOD(error_exception, getSeverity)

DEFAULT_0_PARAMS;

ZVAL_COPY(return_value, GET_PROPERTY(getThis(), ZEND_STR_SEVERITY));
ZVAL_COPY(return_value, GET_PROPERTY(&EX(This), ZEND_STR_SEVERITY));
}
/* }}} */

Expand Down Expand Up @@ -610,7 +610,7 @@ ZEND_METHOD(exception, getTraceAsString)

DEFAULT_0_PARAMS;

object = getThis();
object = &EX(This);
base_ce = i_get_exception_base(object);

trace = zend_read_property_ex(base_ce, object, ZSTR_KNOWN(ZEND_STR_TRACE), 1, &rv);
Expand Down Expand Up @@ -643,7 +643,7 @@ ZEND_METHOD(exception, getPrevious)

DEFAULT_0_PARAMS;

ZVAL_COPY(return_value, GET_PROPERTY_SILENT(getThis(), ZEND_STR_PREVIOUS));
ZVAL_COPY(return_value, GET_PROPERTY_SILENT(&EX(This), ZEND_STR_PREVIOUS));
} /* }}} */

/* {{{ proto string Exception|Error::__toString()
Expand All @@ -661,7 +661,7 @@ ZEND_METHOD(exception, __toString)

str = ZSTR_EMPTY_ALLOC();

exception = getThis();
exception = &EX(This);
fname = zend_string_init("gettraceasstring", sizeof("gettraceasstring")-1, 0);

while (exception && Z_TYPE_P(exception) == IS_OBJECT && instanceof_function(Z_OBJCE_P(exception), zend_ce_throwable)) {
Expand Down Expand Up @@ -718,7 +718,7 @@ ZEND_METHOD(exception, __toString)
}
zend_string_release_ex(fname, 0);

exception = getThis();
exception = &EX(This);
/* Reset apply counts */
while (exception && Z_TYPE_P(exception) == IS_OBJECT && (base_ce = i_get_exception_base(exception)) && instanceof_function(Z_OBJCE_P(exception), base_ce)) {
if (Z_IS_RECURSIVE_P(exception)) {
Expand All @@ -729,7 +729,7 @@ ZEND_METHOD(exception, __toString)
exception = GET_PROPERTY(exception, ZEND_STR_PREVIOUS);
}

exception = getThis();
exception = &EX(This);
base_ce = i_get_exception_base(exception);

/* We store the result in the private property string so we can access
Expand Down
16 changes: 8 additions & 8 deletions Zend/zend_generators.c
View file
Expand Up @@ -877,7 +877,7 @@ ZEND_METHOD(Generator, rewind)
return;
}

generator = (zend_generator *) Z_OBJ_P(getThis());
generator = (zend_generator *) Z_OBJ(EX(This));

zend_generator_rewind(generator);
}
Expand All @@ -893,7 +893,7 @@ ZEND_METHOD(Generator, valid)
return;
}

generator = (zend_generator *) Z_OBJ_P(getThis());
generator = (zend_generator *) Z_OBJ(EX(This));

zend_generator_ensure_initialized(generator);

Expand All @@ -913,7 +913,7 @@ ZEND_METHOD(Generator, current)
return;
}

generator = (zend_generator *) Z_OBJ_P(getThis());
generator = (zend_generator *) Z_OBJ(EX(This));

zend_generator_ensure_initialized(generator);

Expand All @@ -936,7 +936,7 @@ ZEND_METHOD(Generator, key)
return;
}

generator = (zend_generator *) Z_OBJ_P(getThis());
generator = (zend_generator *) Z_OBJ(EX(This));

zend_generator_ensure_initialized(generator);

Expand All @@ -959,7 +959,7 @@ ZEND_METHOD(Generator, next)
return;
}

generator = (zend_generator *) Z_OBJ_P(getThis());
generator = (zend_generator *) Z_OBJ(EX(This));

zend_generator_ensure_initialized(generator);

Expand All @@ -978,7 +978,7 @@ ZEND_METHOD(Generator, send)
Z_PARAM_ZVAL(value)
ZEND_PARSE_PARAMETERS_END();

generator = (zend_generator *) Z_OBJ_P(getThis());
generator = (zend_generator *) Z_OBJ(EX(This));

zend_generator_ensure_initialized(generator);

Expand Down Expand Up @@ -1017,7 +1017,7 @@ ZEND_METHOD(Generator, throw)

Z_TRY_ADDREF_P(exception);

generator = (zend_generator *) Z_OBJ_P(getThis());
generator = (zend_generator *) Z_OBJ(EX(This));

zend_generator_ensure_initialized(generator);

Expand Down Expand Up @@ -1052,7 +1052,7 @@ ZEND_METHOD(Generator, getReturn)
return;
}

generator = (zend_generator *) Z_OBJ_P(getThis());
generator = (zend_generator *) Z_OBJ(EX(This));

zend_generator_ensure_initialized(generator);
if (UNEXPECTED(EG(exception))) {
Expand Down
10 changes: 5 additions & 5 deletions ext/curl/curl_file.c
View file
Expand Up @@ -54,7 +54,7 @@ static void curlfile_ctor(INTERNAL_FUNCTION_PARAMETERS)
Create the CURLFile object */
ZEND_METHOD(CURLFile, __construct)
{
return_value = getThis();
return_value = &EX(This);
curlfile_ctor(INTERNAL_FUNCTION_PARAM_PASSTHRU);
}
/* }}} */
Expand All @@ -75,7 +75,7 @@ static void curlfile_get_property(char *name, size_t name_len, INTERNAL_FUNCTION
if (zend_parse_parameters_none() == FAILURE) {
return;
}
res = zend_read_property(curl_CURLFile_class, getThis(), name, name_len, 1, &rv);
res = zend_read_property(curl_CURLFile_class, &EX(This), name, name_len, 1, &rv);
ZVAL_COPY_DEREF(return_value, res);
}

Expand All @@ -87,7 +87,7 @@ static void curlfile_set_property(char *name, size_t name_len, INTERNAL_FUNCTION
Z_PARAM_STR(arg)
ZEND_PARSE_PARAMETERS_END();

zend_update_property_string(curl_CURLFile_class, getThis(), name, name_len, ZSTR_VAL(arg));
zend_update_property_string(curl_CURLFile_class, &EX(This), name, name_len, ZSTR_VAL(arg));
}

/* {{{ proto string CURLFile::getFilename()
Expand Down Expand Up @@ -134,8 +134,8 @@ ZEND_METHOD(CURLFile, setPostFilename)
Unserialization handler */
ZEND_METHOD(CURLFile, __wakeup)
{
zend_unset_property(curl_CURLFile_class, getThis(), "name", sizeof("name")-1);
zend_update_property_string(curl_CURLFile_class, getThis(), "name", sizeof("name")-1, "");
zend_unset_property(curl_CURLFile_class, &EX(This), "name", sizeof("name")-1);
zend_update_property_string(curl_CURLFile_class, &EX(This), "name", sizeof("name")-1, "");
zend_throw_exception(NULL, "Unserialization of CURLFile instances is not allowed", 0);
}
/* }}} */
Expand Down

5 comments on commit c6ad0b9

@nikic
Copy link
Member

@nikic nikic commented on c6ad0b9 Nov 14, 2018 •
edited

Choose a reason for hiding this comment

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

I'd really prefer to keep this at getThis(), which has been the standard way of accessing $this since forever and does not rely on implementation details -- the next time we want to change something here, we're going to have to update large amounts of extension code where this access is now hardcoded.

We can drop the null check from getThis() and add a separate macro (e.g. tryGetThis()) for the much rarer case where $this might not be set.

@dstogov
Copy link
Member Author

Choose a reason for hiding this comment

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

@nikic I agree, getThis() looked better than EX(This).

Now, I replaced almost all getThis() with useless check by EX(This), but we have a lot of places where we still need getThis() with check. Changing the behavior of getThis() + additional macro is a bad idea, I think.

I suppose, we need a good macro name for getThis() without check, or getThisEx(do_check), or add argument for getThis() macro (BC break), or something else. Your ideas are welcome...

@nikic
Copy link
Member

@nikic nikic commented on c6ad0b9 Nov 15, 2018

Choose a reason for hiding this comment

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

Okay, I can see how changing the behavior of getThis() is probably not a good idea. We might use this opportunity to move to new macros with more standard naming, e.g. ZEND_THIS() for just getting &EX(This) and something like ZEND_THIS_OR_NULL(), ZEND_TRY_THIS(), ZEND_MAYBE_THIS() or similar for the case where it may not be set.

@dstogov
Copy link
Member Author

Choose a reason for hiding this comment

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

ZEND_THIS() without check, ZEND_GET_THIS() with check and keep getThis() for compatibility. OK?

@dstogov
Copy link
Member Author

Choose a reason for hiding this comment

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

@nikic ^^

Please sign in to comment.