Skip to content

Declare a number of functions as variadic.#570

Closed
realityking wants to merge 1 commit intophp:PHP-5.6from
realityking:variadic_functions
Closed

Declare a number of functions as variadic.#570
realityking wants to merge 1 commit intophp:PHP-5.6from
realityking:variadic_functions

Conversation

@realityking
Copy link
Copy Markdown
Contributor

Replacement for #553, this time against PHP-5.6.

In the past there there were a number of workarounds used to signify variadic function. Now that we have a syntax for variadics and reflection support, it's time to update those functions. This covers all functions in core.

cc @nikic

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 doesn't look quite right - shouldn't this still use ZEND_BEGIN_ARG_INFO_EX(arginfo_min, 0, 0, 1) to signify that there is one required parameter?

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.

Oh, I see. -1 required args is the same as requiring all args. I'd still keep the explicit form though...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well the majority of functions uses ZEND_BEGIN_ARG_INFO when possible so it seems like to cleaner way to go. Might be even better if we loose the now useless ,0in PHP6.

@nikic
Copy link
Copy Markdown
Member

nikic commented Feb 15, 2014

Merged as 417dbfb. (Without the ARG_INFO_EX -> ARG_INFO changes).

Thanks for the contribution and sorry it took so long to get this merged :/

@realityking
Copy link
Copy Markdown
Contributor Author

Thank you. I'm closing this, might use the parts you didn't merge for another pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants