Skip to content

fake closure comparison#7223

Closed
krakjoe wants to merge 9 commits into
php:masterfrom
krakjoe:fake-closure-compare
Closed

fake closure comparison#7223
krakjoe wants to merge 9 commits into
php:masterfrom
krakjoe:fake-closure-compare

Conversation

@krakjoe
Copy link
Copy Markdown
Member

@krakjoe krakjoe commented Jul 8, 2021 •
edited
Loading

@krakjoe krakjoe added the Feature label Jul 8, 2021
nikic
nikic reviewed Jul 8, 2021
View reviewed changes
Comment thread Zend/zend_closures.c Outdated
Comment thread Zend/zend_closures.c Outdated
Comment thread Zend/zend_closures.c Outdated
patrickallaert
patrickallaert reviewed Jul 8, 2021
View reviewed changes
Comment thread Zend/tests/closure_compare.phpt Outdated
@krakjoe krakjoe force-pushed the fake-closure-compare branch 6 times, most recently from 7c3a7b9 to 9474ae5 Compare July 8, 2021 21:04
krakjoe
krakjoe commented Jul 8, 2021
View reviewed changes
Comment thread Zend/zend_closures.c Outdated
bwoebi
bwoebi reviewed Jul 8, 2021
View reviewed changes
Comment thread Zend/zend_closures.c Outdated
@krakjoe krakjoe force-pushed the fake-closure-compare branch 4 times, most recently from 31f6efa to 6e32816 Compare July 8, 2021 21:24
@krakjoe krakjoe force-pushed the fake-closure-compare branch from 6e32816 to d600213 Compare July 9, 2021 07:02
@krakjoe krakjoe requested review from bwoebi and nikic July 9, 2021 07:04
nikic
nikic reviewed Jul 9, 2021
View reviewed changes
Comment thread Zend/zend_closures.c
Comment thread Zend/zend_closures.c Outdated
Comment thread Zend/zend_closures.c Outdated
Comment thread Zend/zend_closures.c Outdated
nikic
nikic reviewed Jul 9, 2021
View reviewed changes
Comment thread Zend/zend_closures.c Outdated
nikic
nikic reviewed Jul 9, 2021
View reviewed changes
Comment thread Zend/tests/closure_compare.phpt Outdated
@nikic
Copy link
Copy Markdown
Member

nikic commented Jul 9, 2021

Implementation looks okay, but I think this will need a mail to internals.

TysonAndre
TysonAndre previously requested changes Jul 9, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@TysonAndre TysonAndre left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think $closure == $closure should always return true for the same object.

Also, this should test the expected behavior of closures with static variables

EDIT: Never mind, PHP's == uses zend_compare_objects, which has that check already

Comment thread Zend/zend_closures.c
Comment thread Zend/zend_closures.c
@nikic nikic added this to the PHP 8.1 milestone Jul 12, 2021
@krakjoe
Copy link
Copy Markdown
Member Author

krakjoe commented Jul 13, 2021

Merged as 6a9daaf

@krakjoe krakjoe closed this Jul 13, 2021
@krakjoe krakjoe deleted the fake-closure-compare branch July 13, 2021 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TysonAndre TysonAndre TysonAndre left review comments

@bwoebi bwoebi Awaiting requested review from bwoebi

+2 more reviewers

@patrickallaert patrickallaert patrickallaert approved these changes

@nikic nikic nikic approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature

Projects

None yet

Milestone

PHP 8.1

Development

Successfully merging this pull request may close these issues.

5 participants

@krakjoe @nikic @patrickallaert @TysonAndre @bwoebi