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

ext/curl: Refactor cURL to only use FCC #13291

Merged
merged 15 commits into from May 1, 2024

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Jan 31, 2024

Refactor cURL options that use callables to check them on assignment and use the standard FCC instead of its custom struct.

  • Finish conversions
  • Tests for trampolines as callables
  • Test multi handler (with normal callables and trampoline)

ext/curl/multi.c Outdated Show resolved Hide resolved
ext/curl/interface.c Outdated Show resolved Hide resolved
ext/curl/interface.c Outdated Show resolved Hide resolved
ext/curl/interface.c Outdated Show resolved Hide resolved
ext/curl/interface.c Outdated Show resolved Hide resolved
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Two minor comments otherwise looks good

efree(mh->handlers.server_push);

if (ZEND_FCC_INITIALIZED(mh->handlers.server_push)) {
zend_fcc_dtor(&mh->handlers.server_push);
Copy link
Member

Choose a reason for hiding this comment

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

In other places you set it then to the empty fcc, did you forget to do that here?

case CURLMOPT_PUSHFUNCTION: {
/* See php_curl_set_callable_handler */
if (ZEND_FCC_INITIALIZED(mh->handlers.server_push)) {
zend_fcc_dtor(&mh->handlers.server_push);
Copy link
Member

Choose a reason for hiding this comment

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

Should this be set to the empty fcc?

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!
Only 2 remarks I have is one that I wrote in the comment, and one that I told you already: zend_fcc_dtor actually does set the fcc to empty, so manually overwriting to empty is not necessary (Sorry!).

ext/curl/interface.c Outdated Show resolved Hide resolved
@Girgias Girgias merged commit f4dbe23 into php:master May 1, 2024
9 of 10 checks passed
@Girgias Girgias deleted the curl-fcc-refactoring branch May 1, 2024 14:09
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.

None yet

2 participants