ext/reflection: various cleanups and simplifications (2) by DanielEScherzer · Pull Request #22660 · php/php-src

DanielEScherzer · 2026-07-09T19:53:11Z

No description provided.

Given that the `Closure` class is declared as `final`, for some class entry `ce` to represent an instance of a `Closure` it must be exactly `zend_ce_closure`. Replace the `instanceof_function()` call with a simple pointer comparison.

While different callers provided different levels of indentation to the function, the indentation was never used. Remove the parameter and update callers.

…mon` Internal and userland functions store their documentation comments the same way, as a `zend_string` pointer. These pointers are part of the common elements at the start of both `zend_op_array` and `zend_internal_function` that are made available via `zend_function.common`; use the common access rather than checking for the different types of functions individually.

…ng()` For dynamic properties all of the representation details are known immediately, rather than building up the string in pieces (first the indented `Property [ `, then the `<dynamic> public` followed by the name, and finally the closing ` ]\n`) use a single `smart_str_append_printf()` call and an early return.

When a variable has a single consistent initialization location, move the declaration to be combined with the initialization.

Move variable declarations into the loop start macro where possible to have the types of the variables clear in the loop.

If they cannot be declared at the point of initialization, at least move the declarations closer to where the variables are used.

Consistently indent with tabs, split up some long lines, combine some nested conditions, and overall try to improve readability.

Girgias

Did the review commit by commit. And found some orthogonal issues that can be fixed here or in a follow-up PR.

Girgias · 2026-07-10T11:58:45Z

 				ce = Z_OBJCE_P(reference);

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


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.

Girgias · 2026-07-10T11:59:15Z

 	} ZEND_HASH_FOREACH_END();

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


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

Girgias · 2026-07-10T12:02:40Z

+	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);


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.

Girgias · 2026-07-10T12:04:45Z

 	object_init_ex(object, reflection_attribute_ptr);
-	intern  = Z_REFLECTION_P(object);
-	reference = (attribute_reference*) emalloc(sizeof(attribute_reference));
+	reflection_object *intern  = Z_REFLECTION_P(object);


Nit: two spaces (or tabs?) after the variable name and the = sign which are not aligned anyway.

Girgias · 2026-07-10T12:06:23Z

+	zval *object = ZEND_THIS;
+	reflection_object *intern = Z_REFLECTION_P(object);


Is the object zval even used somewhere else? Might be better to just do Z_REFLECTION_P(ZEND_THIS);

Girgias · 2026-07-10T12:11:38Z

 		if (ini_entry->modifiable == ZEND_INI_ALL) {
 			smart_str_appends(str, "ALL");
 		} else {
+			char *comma = "";


Could be const.

Girgias · 2026-07-10T12:12:54Z

 		}

+		zval *params;
+		int num_args;


Should be uint32_t

Girgias · 2026-07-10T12:13:10Z

 		RETURN_THROWS();
 	}

+	int argc = 0;


Should be uint32_t

Girgias · 2026-07-10T12:14:09Z

+	zend_class_entry *ce;
 	if (NULL == (ce = zend_lookup_class(attr->data->name))) {


Could be rewritten as:

zend_class_entry *ce = zend_lookup_class(attr->data->name); if (ce == NULL) {

Girgias · 2026-07-10T12:27:14Z

+		zend_string *mname = zend_string_alloc(ZSTR_LEN(class_name) + ZSTR_LEN(cur_ref->method_name) + 2, false);
+		snprintf(ZSTR_VAL(mname), ZSTR_LEN(mname) + 1, "%s::%s", ZSTR_VAL(class_name), ZSTR_VAL(cur_ref->method_name));


Aside: This feels this would be safer if it was a simple zend_strpprintf.

DanielEScherzer added 11 commits July 8, 2026 19:42

Fix inline documentation of ReflectionParameter::isPassedByReference()

6ea723a

Reflection: remove unused indentation support from _parameter_string()

d363409

While different callers provided different levels of indentation to the function, the indentation was never used. Remove the parameter and update callers.

Reflection: start cleaning up variable declarations

4856fb9

When a variable has a single consistent initialization location, move the declaration to be combined with the initialization.

Reflection: clean up variables for hash iteration

c42748d

Move variable declarations into the loop start macro where possible to have the types of the variables clear in the loop.

Reflection: unhoist more variable declarations

ba403df

If they cannot be declared at the point of initialization, at least move the declarations closer to where the variables are used.

Reflection: layout and indentation cleanup

75064bb

Consistently indent with tabs, split up some long lines, combine some nested conditions, and overall try to improve readability.

Reflection: use early returns to reduce indentation

6e5dc76

github-actions Bot added the Extension: reflection label Jul 9, 2026

Girgias reviewed Jul 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

ext/reflection: various cleanups and simplifications (2)#22660
DanielEScherzer wants to merge 11 commits into
php:masterfrom
DanielEScherzer:reflection-cleanup-202607-r2

DanielEScherzer commented Jul 9, 2026

Uh oh!

Girgias left a comment

Uh oh!

Girgias Jul 10, 2026

Uh oh!

Girgias Jul 10, 2026

Uh oh!

Girgias Jul 10, 2026

Uh oh!

Girgias Jul 10, 2026

Uh oh!

Girgias Jul 10, 2026

Uh oh!

Girgias Jul 10, 2026

Uh oh!

Girgias Jul 10, 2026

Uh oh!

Girgias Jul 10, 2026

Uh oh!

Girgias Jul 10, 2026

Uh oh!

Girgias Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		zval *object = ZEND_THIS;
		reflection_object *intern = Z_REFLECTION_P(object);

		zend_class_entry *ce;
		if (NULL == (ce = zend_lookup_class(attr->data->name))) {

		zend_string *mname = zend_string_alloc(ZSTR_LEN(class_name) + ZSTR_LEN(cur_ref->method_name) + 2, false);
		snprintf(ZSTR_VAL(mname), ZSTR_LEN(mname) + 1, "%s::%s", ZSTR_VAL(class_name), ZSTR_VAL(cur_ref->method_name));

Uh oh!

Conversation

DanielEScherzer commented Jul 9, 2026

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants