FFI::CType reflection API #7217

dstogov · 2021-07-06T03:19:14Z

No description provided.

nikic

This looks reasonable to me, don't have anything to add in terms of implementation.

Just some bikeshedding on method names.

ext/ffi/ffi.stub.php

except for "getPointerType" -> "getPointerElementType"

dstogov · 2021-07-06T09:18:44Z

Just some bikeshedding on method names.

I changes names according to your suggestion, except for getPointerType. (getPointerElementType looks wrong to me)

lisachenko

Hello, @dstogov! Thank you for this improvement! Looks awesome!

I have only small suggestions regarding better method names, which will be used better.

Also, want to avoid some specific methods for CType that can be implemented better as children classes, eg CArrayType, CStructureType. But this can require some additional changes... @dstogov @nikic what do you think?

ext/ffi/ffi.c

lisachenko · 2021-07-06T12:36:16Z

ext/ffi/ffi.c

+}
+/* }}} */
+
+ZEND_METHOD(FFI_CType, getArrayElementType) /* {{{ */


What do you think about extraction of separate CArrayType for this? It can make API stable and more logical, as there is no point in calling getArrayElementType for simple scalar types - it will always throw an error, which can be example of LSP principle - we will have methods that won't work.

In general, it's possible to introduce and instantiate CArrayType instead of CType, but I don't like to do this in last minute and without RFC.

lisachenko · 2021-07-06T12:37:16Z

ext/ffi/ffi.c

+
+	type = ZEND_FFI_TYPE(ctype->type);
+	if (type->kind != ZEND_FFI_TYPE_ARRAY) {
+		zend_throw_error(zend_ffi_exception_ce, "FFI\\CType is not an array");


This runtime check can be avoid by moving implementation into the separate CArrayType

lisachenko · 2021-07-06T12:37:58Z

ext/ffi/ffi.c

+}
+/* }}} */
+
+ZEND_METHOD(FFI_CType, getPointerType) /* {{{ */


Same here, we can have separate CPointerType, which will give us better visibility in PHP code.

ext/ffi/ffi.c

lisachenko · 2021-07-06T12:46:52Z

ext/ffi/ffi.c

+}
+/* }}} */
+
+ZEND_METHOD(FFI_CType, getFuncArgType) /* {{{ */


getFunctionArgumentType()?

in my opinion, this longer name doesn't make things more clear.

PHP is already already using "Parameter" instead of "Argument" for reflection on types elsewhere.

Also, would omitting Func for something like getParameterType make more sense - nothing other than functions would have parameters.

lisachenko · 2021-07-06T12:47:34Z

ext/ffi/ffi.c

+	zend_ffi_ctype *ret;
+
+	ZEND_PARSE_PARAMETERS_START(1, 1)
+		Z_PARAM_LONG(n)


argumentIndex?

This makes sense, but not in this place but in stub, and probably $arg_index.

lisachenko · 2021-07-06T12:49:39Z

ext/ffi/ffi.c

+	}
+
+	if (!type->func.args) {
+		zend_throw_error(zend_ffi_exception_ce, "Wrong argument number");


Maybe state explicitly that "Function $name has not arguments".

May be this makes sense, however, we don't know the function name at this point.

Ok, then current one should be ok

Suggested change

zend_throw_error(zend_ffi_exception_ce, "Wrong argument number");

zend_throw_error(zend_ffi_exception_ce, "Missing parameter offset #d of FFI function with %d parameter(s)", ...);

Maybe something more along these lines with the argument count and offset being fetched, to speed up debugging? (my wording can be improved)

ext/ffi/ffi.c

lisachenko · 2021-07-06T12:55:21Z

ext/ffi/ffi.stub.php

+    public function getFuncABI(): int {}
+    public function getFuncReturnType(): CType {}
+    public function getFuncArgCount(): int {}
+    public function getFuncArgType(int $n): CType {}


Suggested change

public function getFuncArgType(int $n): CType {}

public function getFunctionArgumentType(int $argumentIndex): CType {}

lisachenko · 2021-07-06T13:03:37Z

As I can see method names have been partially updated already.

nikic · 2021-07-06T13:17:49Z

Also, want to avoid some specific methods for CType that can be implemented better as children classes, eg CArrayType, CStructureType. But this can require some additional changes... @dstogov @nikic what do you think?

I agree that using subclasses would be cleaner in principle, but I'm also okay with the current approach.

nikic · 2021-07-06T13:22:21Z

Just some bikeshedding on method names.

I changes names according to your suggestion, except for getPointerType. (getPointerElementType looks wrong to me)

Ah, I probably picked up an LLVM-ism, where this is called "pointer element type" (or sometimes "pointee type"). I think just keeping getPointerType() is fine.

TysonAndre

LGTM despite nits on naming - I think it'd be easier to remember this if names were consistent with Reflection APIs already in PHP such as https://www.php.net/manual/en/class.reflectionfunctionabstract.php

TysonAndre · 2021-07-07T13:14:56Z

ext/ffi/ffi.c

+}
+/* }}} */
+
+ZEND_METHOD(FFI_CType, getArrayLength) /* {{{ */


Would implementing Countable and calling this Countable::count() make sense here (continuing to throw for non-arrays)? for ($i = 0; $i < count($ffiArray); $i++) {...}

Then again, that'd be a much larger change and that interface isn't in FFI\CData for 8.0

TysonAndre · 2021-07-07T13:15:40Z

ext/ffi/ffi.c

+}
+/* }}} */
+
+ZEND_METHOD(FFI_CType, getFuncReturnType) /* {{{ */


Would it make sense to use the same names that reflection already uses for these methods? https://www.php.net/manual/en/reflectionfunctionabstract.getreturntype.php E.g. getReturnType

TysonAndre · 2021-07-07T13:16:35Z

ext/ffi/ffi.c

+}
+/* }}} */
+
+ZEND_METHOD(FFI_CType, getFuncArgCount) /* {{{ */


https://www.php.net/manual/en/reflectionfunctionabstract.getnumberofparameters.php

Alternately, getNumberOfParameters

TysonAndre · 2021-07-07T13:18:06Z

ext/ffi/ffi.c

+}
+/* }}} */
+
+ZEND_METHOD(FFI_CType, getFuncArgType) /* {{{ */


PHP is already already using "Parameter" instead of "Argument" for reflection on types elsewhere.

Also, would omitting Func for something like getParameterType make more sense - nothing other than functions would have parameters.

TysonAndre · 2021-07-07T13:22:25Z

ext/ffi/ffi.c

+	if (zend_parse_parameters_none() == FAILURE) {
+		RETURN_THROWS();
+	}


Suggested change

if (zend_parse_parameters_none() == FAILURE) {

RETURN_THROWS();

}

ZEND_PARSE_PARAMETERS_NONE();

nit: Could be shortened here and elsewhere. Is there a reason for different parts of the codebase using different ways of writing this?

TysonAndre · 2021-07-07T13:27:05Z

ext/ffi/ffi.c

+	}
+
+	if (!type->func.args) {
+		zend_throw_error(zend_ffi_exception_ce, "Wrong argument number");


Suggested change

zend_throw_error(zend_ffi_exception_ce, "Wrong argument number");

zend_throw_error(zend_ffi_exception_ce, "Missing parameter offset #d of FFI function with %d parameter(s)", ...);

Maybe something more along these lines with the argument count and offset being fetched, to speed up debugging? (my wording can be improved)

TysonAndre · 2021-07-07T13:35:38Z

Elsewhere in php,

Parameter is used to refer to the function declarations (AST_PARAM, ReflectionFunctionAbstract->getParameters(), etc.)
Argument is used to refer to expressions passed to the functions by the caller (ArgumentCountError, etc.)

Other languages use similar definitions, e.g. https://developer.mozilla.org/en-US/docs/Glossary/Parameter

dstogov · 2021-07-13T10:16:00Z

Merged via a2845e3

SerafimArts · 2021-07-22T14:11:33Z

I understand that it is already merged, but won't $type->getKind() return different values every time in case of different settings of the HAVE_LONG_DOUBLE definition: https://github.com/php/php-src/blob/master/ext/ffi/ffi.c#L69?

$type = \FFI::type('uint8_t');

// #define HAVE_LONG_DOUBLE 1
var_dump($type->getKind()); // 4

// #define HAVE_LONG_DOUBLE 0
var_dump($type->getKind()); // 3

That is, we cannot be guided by these values with complete confidence. Maybe it makes sense to add a set of constants like?

namespace FFI;

class CType 
{
    public const TYPE_KIND_VOID = // ZEND_FFI_TYPE_VOID;
    public const TYPE_KIND_FLOAT = // ZEND_FFI_TYPE_FLOAT;
    // etc...
}

The same goes for other enams defined in ffi.c

nikic · 2021-07-22T14:16:08Z

@SerafimArts These constants are already present:

php-src/ext/ffi/ffi.c

Line 5357 in e1f211f

REGISTER_FFI_TYPE_CONSTANT(TYPE_VOID);

SerafimArts · 2021-07-22T14:22:37Z

@nikic Oh, then I'm sorry, I didn't notice

dstogov added 3 commits July 6, 2021 06:16

FFI::CType reflection API

b040e24

Fixed refcounting

070c48f

Fixed return type namespaces

e8265f1

nikic reviewed Jul 6, 2021

View reviewed changes

ext/ffi/ffi.stub.php Outdated Show resolved Hide resolved

ext/ffi/ffi.stub.php Outdated Show resolved Hide resolved

ext/ffi/ffi.stub.php Show resolved Hide resolved

ext/ffi/ffi.stub.php Outdated Show resolved Hide resolved

ext/ffi/ffi.stub.php Outdated Show resolved Hide resolved

Follow to Nikita's method naming suggestions,

779ff7b

except for "getPointerType" -> "getPointerElementType"

lisachenko reviewed Jul 6, 2021

View reviewed changes

Use better argument names

c580ae5

lisachenko approved these changes Jul 7, 2021

View reviewed changes

TysonAndre reviewed Jul 7, 2021

View reviewed changes

dstogov closed this Jul 13, 2021

TysonAndre mentioned this pull request Jul 13, 2021

Proposal: Rename FFI getFuncArg* to getFuncParameter*, $arg_index->$index #7236

Merged

SerafimArts mentioned this pull request Jul 22, 2021

Added PHP 8.1 FFI API support JetBrains/phpstorm-stubs#1183

Merged

cmb69 mentioned this pull request Dec 21, 2021

Add undocumented FFI\CType methods php/doc-en#1227

Merged

SerafimArts mentioned this pull request Jun 27, 2022

[VarDumper] Add FFI\CData and FFI\CType types symfony/symfony#46773

Merged

4 tasks

	zend_throw_error(zend_ffi_exception_ce, "Wrong argument number");
	zend_throw_error(zend_ffi_exception_ce, "Missing parameter offset #d of FFI function with %d parameter(s)", ...);

	public function getFuncArgType(int $n): CType {}
	public function getFunctionArgumentType(int $argumentIndex): CType {}

FFI::CType reflection API #7217

FFI::CType reflection API #7217

Uh oh!

Conversation

dstogov commented Jul 6, 2021

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dstogov commented Jul 6, 2021

Uh oh!

lisachenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TysonAndre Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lisachenko commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Jul 6, 2021

Uh oh!

nikic commented Jul 6, 2021

Uh oh!

TysonAndre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TysonAndre Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TysonAndre Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TysonAndre commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TysonAndre Jul 7, 2021 •

edited

Loading

lisachenko commented Jul 6, 2021 •

edited

Loading

TysonAndre Jul 7, 2021 •

edited

Loading

TysonAndre Jul 7, 2021 •

edited

Loading

TysonAndre commented Jul 7, 2021 •

edited

Loading

SerafimArts commented Jul 22, 2021 •

edited

Loading

SerafimArts commented Jul 22, 2021 •

edited

Loading