-
Notifications
You must be signed in to change notification settings - Fork 399
extgen: (types) make go arrays more consistent with PHP arrays #1800
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the new interface, but I feel like it adds more restrictions than it removes. For example, you're most likely going to be using this "on the edge" so you are just going to be iterating over a slice or map and making it into an array or vice-versa.
A better experience would just be wrapping a Map for an associative array, with some behavior and allowing the dev to access that Map. That would save having to convert it twice...
I would suggest doing the same for slices. Maybe even making a marshaler to/from json -- I already have a rudimentary one I could share.
I don't think it makes sense to make the Array type actually usable, but just behavior to convert from go-ism to php-ism and back again.
|
Converting either to a For packed arrays you have direct access to the slice via Easiest solution is to just ditch the order, i can reset it back to that implementation if the general consensus is that order doesn't matter that much. |
|
Maybe it could be nice to support ordered associative arrays, and typical unordered Go maps? |
|
Could do it like this? type OrderedMap struct {
keys []string
data map[string]interface{}
}It still allows ordered access by iterating over keys, and O(1) lookup by key via the map. |
|
For full context, I'm also working on an implementation that changes this whole thing up. Right now, it basically copy-pastas your implementation into a new file. Instead of doing that, the implementation I'm working on would allow you to write code like this: //export_php:function foo(string $value): void
func foo(val string) {}Then in the generated file, instead of reusing the same package name and copy-pasting your implementation, it will generate a new package ("ext" by default): package ext
//export
func go_foo(value *C.zend_string) {
foo(frankenphp.GoString(value))
}This makes it so that:
So, it is unlikely anyone will ever use our internal types directly using the generator. That's why I said having direct access to the maps/slices will probably be better. |
|
Yeah we can also ditch the types completely and just have something like PHPAssociativeArray(entries map[string]interface{}, order []string) unsafe.Pointer
PHPPackedArray(values []interface{}) unsafe.Pointer
GoAssociativeArray(phpArray unsafe.Pointer, ordered bool) (map[string]interface{}, []string)
GoPackedArray(phpArray unsafe.Pointer) []interface{} |
|
Since we expect the type (*C.zval), it would be great to just accept/return that type instead of unsafe.Pointer. I already got bit a couple of times because I changed my type, but not the function to transform it, and unsafe.Pointer doesn't cause a compilation error. |
|
cgo types cannot be exported/used by another package. This causes runtime issues, we already got beat by that. What we may do however, is creating to dedicated aliases of |
|
Alright, there are now multiple ways to convert an array to cover all 3 cases with direct access to the primitives on the go side:
type AssociativeArray struct {
Map map[string]interface{}
Order []string
}
func GoAssociativeArray(arr unsafe.Pointer, ordered bool) (AssociativeArray, error)
func PHPAssociativeArray(arr AssociativeArray) unsafe.Pointer
type PackedArray = []interface{}
func GoPackedArray(arr unsafe.Pointer) (PackedArray, error)
func PHPPackedArray(arr PackedArray) unsafe.Pointer |
|
I wonder if having |
|
Yeah we can do that as well 👍 . We could also do something like this: func GoMap(unsafe.Pointer) map[string]any
func PHPMap(map[string]any) unsafe.Pointer
func GoSlice(unsafe.Pointer) []any
func PHPSlice([]any) unsafe.Pointer
func GoArray(unsafe.Pointer) AssociativeArray
func PHPArray(AssociativeArray) unsafe.Pointer |
|
LGTM! I would just go for |
|
Thank you!! |
* Makes go arrays more consistent with PHP arrays. * NewAssociativeArray. * linting * go linting * Exposes all primitive types. * Removes pointer alias * linting * Optimizes hash update. * Fixes extgen tests. * Moves file to tests. * Fixes suggested by @dunglas. * Replaces 'interface{}' with 'any'. * Panics on wrong zval. * interface improvements as suggested by @dunglas. * Adjusts docs. * Adjusts docs. * Removes PackedArray alias and adjusts docs. * Updates docs.
Related to #1771
I noticed that the go representation of an array is not fully consistent with the PHP representation.
Current downfalls of
Array:So I ended up doing more work than I originally anticipated to create a data structure that would both represent the packed and unpacked arrays.
Arrayis now essentially a minimal implementation of an ordered map with a fallback to a slice if the array is packed. Users can use array.Values() or array.Entries() which are optimized for the packed or associated arrays respectively.This PR also includes a new thread type for testing the implementation (currently test-only)
cc @alexandre-daubois