Skip to content
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.

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

Fixed some arg infos to match documentation #4139

Closed
wants to merge 1 commit into from

Conversation

kukulich
Copy link
Contributor

@kukulich kukulich commented May 9, 2019

No description provided.

@KalleZ
Copy link
Member

KalleZ commented May 9, 2019

Hi, this should target PHP-7.4 as a bare minimum, thank you :)

@kukulich kukulich changed the base branch from master to PHP-7.4 May 9, 2019 20:10
@petk petk added the Quickfix label May 11, 2019
@nikic
Copy link
Member

nikic commented May 13, 2019

========DIFF========
005+     Parameter #1 [ <optional> $flags ]
005-     Parameter #1 [ <optional> $ar_flags ]
========DONE========
FAIL Bug#71412 ArrayIterator reflection parameter info [ext/spl/tests/bug71412.phpt] 

@nikic
Copy link
Member

nikic commented May 13, 2019

Apart from the test failure those renames look fine. I think we should also update the proto comments to match though.

@kukulich kukulich force-pushed the arg-info branch 2 times, most recently from 5cffd47 to 0fe767d Compare May 13, 2019 09:27
@kukulich
Copy link
Contributor Author

@nikic I hope it's ok now.

ext/spl/spl_array.c Show resolved Hide resolved
ext/spl/spl_fixedarray.c Show resolved Hide resolved
ext/spl/spl_observer.c Show resolved Hide resolved
ZEND_ARG_INFO(0, a)
ZEND_ARG_INFO(0, b)
ZEND_ARG_INFO(0, value1)
ZEND_ARG_INFO(0, value2)
Copy link
Member

Choose a reason for hiding this comment

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

Should update

/* {{{ proto bool SplMinHeap::compare(mixed $a, mixed $b)
and friends.

@kukulich
Copy link
Contributor Author

I hope it's finally ok :) I'm sorry it's my first PR to PHP source and I'm only a PHP/Javascript developer :)

@nikic
Copy link
Member

nikic commented May 22, 2019

Sorry for the delay, now merged as d6c0c5e.

@nikic nikic closed this May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants