Skip to content

Conversation

@Girgias
Copy link
Member

@Girgias Girgias commented Aug 14, 2023

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

LGTM otherwise, thank you @Girgias!

static void zend_type_copy_ctor(zend_type *type, bool use_arena, bool persistent);

static void zend_type_list_copy_ctor(
zend_type *const global_type,
Copy link
Member

Choose a reason for hiding this comment

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

parent_type? Or maybe list_type. Global type sounds like it's the root element.

ZEND_ASSERT(ZEND_TYPE_HAS_NAME(*list_type));
zend_string_addref(ZEND_TYPE_NAME(*list_type));
} ZEND_TYPE_LIST_FOREACH_END();
zend_type_list_copy_ctor(type, ZEND_TYPE_LIST(*type), use_arena, persistent);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: No reason to pass ZEND_TYPE_LIST(*type) instead of accessing it from the function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right, that's a remnant from an earlier iteration of the function

 - phpGH-11958: DNF types in trait properties do not get bound properly
 - phpGH-11883: Memory leak in zend_type_release() for non-arena allocated DNF types
 - Internal trait bound to userland class would not be arena allocated
 - Property DNF types were not properly deep copied during lazy loading

Co-authored-by: Ilija Tovilo <ilija.tovilo@me.com>
Co-authored-by: ju1ius <jules.bernable@gmail.com>
@Girgias Girgias force-pushed the dnf-types-bugfixes branch from 34d2cf9 to 02a80c5 Compare August 15, 2023 14:34
@Girgias Girgias merged commit 02a80c5 into php:PHP-8.2 Aug 15, 2023
@Girgias Girgias deleted the dnf-types-bugfixes branch August 15, 2023 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants