Skip to content

FFI extension add FFI\CDef class and other update #8585

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

Closed
wants to merge 33 commits into from

Conversation

chopins
Copy link
Contributor

@chopins chopins commented May 20, 2022

Update:

  1. Change FFI::cdef(),FFI::load(),FFI::scope() return FFI\CDef instead of FFI object, call C function by FFI\CDef object
  2. addFFI::hasSymbol(FFI\CDef $cdef, string $symbol, ?int $symbol_kind = null):bool FFI::getSymbols(FFI\CDef $cdef, ?int $symbol_kind = null):array[FFI\CType] methods
  3. add FFI::SYM_TYPE, FFI::SYM_CONST,FFI::SYM_VAR,FFI::SYM_FUNC constants
  4. add $cdef option paramter to end of FFI::new(), FFI::cast(),FFI::type()
  5. FFI::new(), FFI::cast(),FFI::type() will be only static method,
  6. FFI non-static method avoid taking up C function names, and not need exclude in zend_ffi_handlers.get_method

see #8554

@bwoebi
Copy link
Member

bwoebi commented May 20, 2022

While I agree that it should have been done this way from the start, I'm a bit unsure about the BC break this implicates.

What's your suggestion how to write version agnostic code, allowing code to be written in PHP 8.0.0 and PHP 8.2.0? For handling class type hints I can polyfill class_alias("FFI", "FFI\\CDef"). But what about the static vs non-static methods (in particular new())? The cdef parameter did not exist beforehand.

@chopins
Copy link
Contributor Author

chopins commented May 20, 2022

  1. I suggestion next release version of 8.1
  2. Now new() cast() type() can be called by two ways (statically and non-statically), this is weird
  3. FFI object non-statically method will conflict with load DLL C function, when they same name. if FFI will adding new non-statically method and must add special handling, and block same C function. this is not best way.
  4. $cdef parameter is optional and is at end, so can append cdef : $cdef for compatible. if be called by statically and no pass it will same as now. if be called by non-statically will invoke C function, old code will report not exists C function error

so class_alias can not resolve above these issues

@ramsey
Copy link
Member

ramsey commented May 21, 2022

I suggestion next release version of 8.1

This cannot go into 8.1. It will need to go into 8.2, if it's accepted.

@ramsey ramsey linked an issue May 21, 2022 that may be closed by this pull request
@cmb69
Copy link
Member

cmb69 commented May 23, 2022

I fully agree with @bwoebi's comment above; while it might better have been done this way from the beginning, I don't think we can change it for BC reasons. At least this should go through the RFC process.

@cmb69 cmb69 added the RFC label May 23, 2022
@chopins
Copy link
Contributor Author

chopins commented Jun 7, 2022

@cmb69
Is need me submit a RFC in https://wiki.php.net/rfc

@cmb69
Copy link
Member

cmb69 commented Jun 7, 2022

Is need me submit a RFC in https://wiki.php.net/rfc

Yes. See https://wiki.php.net/rfc/howto on how to do it.

@chopins
Copy link
Contributor Author

chopins commented Jun 11, 2022

Yes. See https://wiki.php.net/rfc/howto on how to do it.

OK,I'll submit it when I have free time

@chopins
Copy link
Contributor Author

chopins commented Jun 21, 2022

I can not login to php wiki. so i maybe not submit the RFC.
wait other people submit it.

@cmb69
Copy link
Member

cmb69 commented Jun 21, 2022

I can not login to php wiki.

Why? What happens?

There is a user chopins with RFC karma, so login via https://wiki.php.net/?do=login should be possible. If you forgot your password, there is a link "Set new password" on that page.

@chopins chopins requested a review from dstogov as a code owner October 7, 2023 13:51
@chopins chopins requested a review from kocsismate as a code owner April 15, 2024 15:43
@chopins chopins closed this May 20, 2024
@chopins chopins deleted the ffi-cdef-class branch May 20, 2024 08:15
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.

Chage FFI::cdef() return FFI\CDef object
4 participants