Skip to content

ext/reflection: various cleanups and simplifications (2) #22660

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.

Sign up for GitHub

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

Open
DanielEScherzer wants to merge 11 commits into
base: master
Choose a base branch
from
+632 −784
Open

ext/reflection: various cleanups and simplifications (2) #22660

Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6ea723a
Fix inline documentation of `ReflectionParameter::isPassedByReference()`
DanielEScherzer Jul 9, 2026
591514f
`ReflectionParameter::__construct()`: optimize closure checking
DanielEScherzer Jul 9, 2026
f44c8c7
`ReflectionClass::getMethods()`: optimize closure checking
DanielEScherzer Jul 9, 2026
d363409
Reflection: remove unused indentation support from `_parameter_string()`
DanielEScherzer Jul 9, 2026
8bd459e
`ReflectionFunctionAbstract::getDocComment()`: use `zend_function.com…
DanielEScherzer Jul 9, 2026
8d5ba54
Reflection: improve handling of dynamic properties in `_property_stri…
DanielEScherzer Jul 9, 2026
4856fb9
Reflection: start cleaning up variable declarations
DanielEScherzer Jul 9, 2026
c42748d
Reflection: clean up variables for hash iteration
DanielEScherzer Jul 9, 2026
ba403df
Reflection: unhoist more variable declarations
DanielEScherzer Jul 9, 2026
75064bb
Reflection: layout and indentation cleanup
DanielEScherzer Jul 9, 2026
6e5dc76
Reflection: use early returns to reduce indentation
DanielEScherzer Jul 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
171 changes: 84 additions & 87 deletions ext/reflection/php_reflection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -756,7 +756,7 @@ static void format_default_value(smart_str *str, const zval *value) {
}

/* {{{ _parameter_string */
static void _parameter_string(smart_str *str, const zend_function *fptr, const struct _zend_arg_info *arg_info, uint32_t offset, bool required, char* indent)
static void _parameter_string(smart_str *str, const zend_function *fptr, const struct _zend_arg_info *arg_info, uint32_t offset, bool required)
{
smart_str_append_printf(str, "Parameter #%d [ ", offset);
if (!required) {
Expand Down Expand Up @@ -818,7 +818,7 @@ static void _function_parameter_string(smart_str *str, const zend_function *fptr
smart_str_append_printf(str, "%s- Parameters [%d] {\n", indent, num_args);

@Girgias Girgias Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This printf uses the wrong format specifier for num_args btw.

for (i = 0; i < num_args; i++) {
smart_str_append_printf(str, "%s ", indent);
_parameter_string(str, fptr, arg_info, i, i < num_required, indent);
_parameter_string(str, fptr, arg_info, i, i < num_required);
smart_str_appendc(str, '\n');
arg_info++;
}
Expand Down Expand Up @@ -977,88 +977,89 @@ static zval *property_get_default(const zend_property_info *prop_info) {
/* {{{ _property_string */
static void _property_string(smart_str *str, const zend_property_info *prop, const char *prop_name, const char* indent)
{
if (prop && prop->doc_comment) {
if (!prop) {
// Dynamic property, known to have no doc comment, flags, etc.
smart_str_append_printf(str, "%sProperty [ <dynamic> public $%s ]\n", indent, prop_name);
return;
}
if (prop->doc_comment) {
smart_str_append_printf(str, "%s%s\n", indent, ZSTR_VAL(prop->doc_comment));
}
smart_str_append_printf(str, "%sProperty [ ", indent);
Comment on lines +980 to 988

@Girgias Girgias Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it make sense to have a smart_str_appends(str, indent); call before either of the smart_str_append_printf(), as I'm not really the biggest fan of the printf variants personally. But no big opinion here.

if (!prop) {
smart_str_append_printf(str, "<dynamic> public $%s", prop_name);
} else {
if (prop->flags & ZEND_ACC_ABSTRACT) {
smart_str_appends(str, "abstract ");
}
if (prop->flags & ZEND_ACC_FINAL) {
smart_str_appends(str, "final ");
}
/* These are mutually exclusive */
switch (prop->flags & ZEND_ACC_PPP_MASK) {
case ZEND_ACC_PUBLIC:
smart_str_appends(str, "public ");
break;
case ZEND_ACC_PRIVATE:
smart_str_appends(str, "private ");
break;
case ZEND_ACC_PROTECTED:
smart_str_appends(str, "protected ");
break;
}
switch (prop->flags & ZEND_ACC_PPP_SET_MASK) {
case ZEND_ACC_PRIVATE_SET:
smart_str_appends(str, "private(set) ");
break;
case ZEND_ACC_PROTECTED_SET:
smart_str_appends(str, "protected(set) ");
break;
case ZEND_ACC_PUBLIC_SET:
ZEND_UNREACHABLE();
break;
}
if (prop->flags & ZEND_ACC_STATIC) {
smart_str_appends(str, "static ");
}
if (prop->flags & ZEND_ACC_READONLY) {
smart_str_appends(str, "readonly ");
}
if (prop->flags & ZEND_ACC_VIRTUAL) {
smart_str_appends(str, "virtual ");
}
if (ZEND_TYPE_IS_SET(prop->type)) {
zend_string *type_str = zend_type_to_string(prop->type);
smart_str_append(str, type_str);
smart_str_appendc(str, ' ');
zend_string_release(type_str);
}
if (!prop_name) {
const char *class_name;
zend_unmangle_property_name(prop->name, &class_name, &prop_name);
}
smart_str_append_printf(str, "$%s", prop_name);
if (prop->flags & ZEND_ACC_ABSTRACT) {
smart_str_appends(str, "abstract ");
}
if (prop->flags & ZEND_ACC_FINAL) {
smart_str_appends(str, "final ");
}
/* These are mutually exclusive */
switch (prop->flags & ZEND_ACC_PPP_MASK) {
case ZEND_ACC_PUBLIC:
smart_str_appends(str, "public ");
break;
case ZEND_ACC_PRIVATE:
smart_str_appends(str, "private ");
break;
case ZEND_ACC_PROTECTED:
smart_str_appends(str, "protected ");
break;
}
switch (prop->flags & ZEND_ACC_PPP_SET_MASK) {
case ZEND_ACC_PRIVATE_SET:
smart_str_appends(str, "private(set) ");
break;
case ZEND_ACC_PROTECTED_SET:
smart_str_appends(str, "protected(set) ");
break;
case ZEND_ACC_PUBLIC_SET:
ZEND_UNREACHABLE();
break;
}
if (prop->flags & ZEND_ACC_STATIC) {
smart_str_appends(str, "static ");
}
if (prop->flags & ZEND_ACC_READONLY) {
smart_str_appends(str, "readonly ");
}
if (prop->flags & ZEND_ACC_VIRTUAL) {
smart_str_appends(str, "virtual ");
}
if (ZEND_TYPE_IS_SET(prop->type)) {
zend_string *type_str = zend_type_to_string(prop->type);
smart_str_append(str, type_str);
smart_str_appendc(str, ' ');
zend_string_release(type_str);
}
if (!prop_name) {
const char *class_name;
zend_unmangle_property_name(prop->name, &class_name, &prop_name);
}
smart_str_append_printf(str, "$%s", prop_name);

const zval *default_value = property_get_default(prop);
if (default_value && !Z_ISUNDEF_P(default_value)) {
smart_str_appends(str, " = ");
format_default_value(str, default_value);
}
if (prop->hooks != NULL) {
smart_str_appends(str, " {");
const zend_function *get_hooked = prop->hooks[ZEND_PROPERTY_HOOK_GET];
if (get_hooked != NULL) {
if (get_hooked->common.fn_flags & ZEND_ACC_FINAL) {
smart_str_appends(str, " final get;");
} else {
smart_str_appends(str, " get;");
}
const zval *default_value = property_get_default(prop);
if (default_value && !Z_ISUNDEF_P(default_value)) {
smart_str_appends(str, " = ");
format_default_value(str, default_value);
}
if (prop->hooks != NULL) {
smart_str_appends(str, " {");
const zend_function *get_hooked = prop->hooks[ZEND_PROPERTY_HOOK_GET];
if (get_hooked != NULL) {
if (get_hooked->common.fn_flags & ZEND_ACC_FINAL) {
smart_str_appends(str, " final get;");
} else {
smart_str_appends(str, " get;");
}
const zend_function *set_hooked = prop->hooks[ZEND_PROPERTY_HOOK_SET];
if (set_hooked != NULL) {
if (set_hooked->common.fn_flags & ZEND_ACC_FINAL) {
smart_str_appends(str, " final set;");
} else {
smart_str_appends(str, " set;");
}
}
const zend_function *set_hooked = prop->hooks[ZEND_PROPERTY_HOOK_SET];
if (set_hooked != NULL) {
if (set_hooked->common.fn_flags & ZEND_ACC_FINAL) {
smart_str_appends(str, " final set;");
} else {
smart_str_appends(str, " set;");
}
smart_str_appends(str, " }");
}
smart_str_appends(str, " }");
}

smart_str_appends(str, " ]\n");
Expand Down Expand Up @@ -2013,12 +2014,8 @@ ZEND_METHOD(ReflectionFunctionAbstract, getDocComment)

GET_REFLECTION_OBJECT_PTR(fptr);

if (fptr->type == ZEND_USER_FUNCTION && fptr->op_array.doc_comment) {
RETURN_STR_COPY(fptr->op_array.doc_comment);
}

if (fptr->type == ZEND_INTERNAL_FUNCTION && ((zend_internal_function *) fptr)->doc_comment) {
RETURN_STR_COPY(((zend_internal_function *) fptr)->doc_comment);
if (fptr->common.doc_comment) {
RETURN_STR_COPY(fptr->common.doc_comment);
}

RETURN_FALSE;
Expand Down Expand Up @@ -2539,7 +2536,7 @@ ZEND_METHOD(ReflectionParameter, __construct)
case IS_OBJECT: {
ce = Z_OBJCE_P(reference);

if (instanceof_function(ce, zend_ce_closure)) {
if (ce == zend_ce_closure) {

@Girgias Girgias Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might be good to add a comment just to explain it. As instanceof_function() is always inlined to do this pointer comparison and only call the outlined slow function if not true.

fptr = (zend_function *)zend_get_closure_method_def(Z_OBJ_P(reference));
Z_ADDREF_P(reference);
is_closure = true;
Expand Down Expand Up @@ -2635,7 +2632,7 @@ ZEND_METHOD(ReflectionParameter, __toString)

ZEND_PARSE_PARAMETERS_NONE();
GET_REFLECTION_OBJECT_PTR(param);
_parameter_string(&str, param->fptr, param->arg_info, param->offset, param->required, "");
_parameter_string(&str, param->fptr, param->arg_info, param->offset, param->required);
RETURN_STR(smart_str_extract(&str));
}

Expand Down Expand Up @@ -2840,7 +2837,7 @@ ZEND_METHOD(ReflectionParameter, allowsNull)
}
/* }}} */

/* {{{ Returns whether this parameters is passed to by reference */
/* {{{ Returns whether this parameter is passed to by reference */
ZEND_METHOD(ReflectionParameter, isPassedByReference)
{
reflection_object *intern;
Expand Down Expand Up @@ -4550,7 +4547,7 @@ ZEND_METHOD(ReflectionClass, getMethods)
_addmethod(mptr, ce, Z_ARRVAL_P(return_value), filter);
} ZEND_HASH_FOREACH_END();

if (instanceof_function(ce, zend_ce_closure)) {
if (ce == zend_ce_closure) {

@Girgias Girgias Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ditto, probably should be a single commit with all the zend_ce_closure checks rather than multiple.

bool has_obj = Z_TYPE(intern->obj) != IS_UNDEF;
zval obj_tmp;
zend_object *obj;
Expand Down