-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[embind] Add iterable protocol support for bound classes #25993
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
Changes from 8 commits
b65f339
9b8568d
f9752e5
9710f9c
84a572d
841eef4
63d1a41
7662d5a
161b171
3c8594a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -421,6 +421,44 @@ var LibraryEmbind = { | |
| return this.fromWireType({{{ makeGetValue('pointer', '0', '*') }}}); | ||
| }, | ||
|
|
||
| $installIndexedIterator: (proto, sizeMethodName, getMethodName) => { | ||
| const makeIterator = (size, getValue) => { | ||
| #if MEMORY64 | ||
| // size can be either a number or a bigint on wasm64 | ||
| const useBigInt = typeof size === 'bigint'; | ||
| const one = useBigInt ? 1n : 1; | ||
| let index = useBigInt ? 0n : 0; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we simply dictate that interables using I don't image we will ever see 64-bit iterators on wasm32, right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried doing this, but
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case wouldn't we only need to support number here? i.e. if register_vector is using
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i.e. could we just assert
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought it would be better to leave it to the users whether they want size_t/bigint in their container classes or not, since it's fairly trivial to support both for the iterable.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough, but if we want explicitly support this then maybe we should add test for it? Also, should these lines be behind
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Done. |
||
| #else | ||
| let index = 0; | ||
| #endif | ||
| return { | ||
| next() { | ||
| if (index >= size) { | ||
| return { done: true }; | ||
| } | ||
| const current = index; | ||
| #if MEMORY64 | ||
| index += one; | ||
| #else | ||
| index++; | ||
| #endif | ||
| const value = getValue(current); | ||
| return { value, done: false }; | ||
| }, | ||
| [Symbol.iterator]() { | ||
| return this; | ||
| }, | ||
| }; | ||
| }; | ||
|
|
||
| if (!proto[Symbol.iterator]) { | ||
| proto[Symbol.iterator] = function() { | ||
| const size = this[sizeMethodName](); | ||
| return makeIterator(size, (i) => this[getMethodName](i)); | ||
| }; | ||
| } | ||
| }, | ||
|
|
||
| _embind_register_std_string__deps: [ | ||
| '$AsciiToString', '$registerType', | ||
| '$readPointer', '$throwBindingError', | ||
|
|
@@ -1723,6 +1761,19 @@ var LibraryEmbind = { | |
| ); | ||
| }, | ||
|
|
||
| _embind_register_iterable__deps: [ | ||
| '$whenDependentTypesAreResolved', '$installIndexedIterator', '$AsciiToString', | ||
| ], | ||
| _embind_register_iterable: (rawClassType, rawElementType, sizeMethodName, getMethodName) => { | ||
| sizeMethodName = AsciiToString(sizeMethodName); | ||
| getMethodName = AsciiToString(getMethodName); | ||
| whenDependentTypesAreResolved([], [rawClassType, rawElementType], (types) => { | ||
| const classType = types[0]; | ||
| installIndexedIterator(classType.registeredClass.instancePrototype, sizeMethodName, getMethodName); | ||
| return []; | ||
| }); | ||
| }, | ||
|
|
||
| _embind_register_class_constructor__deps: [ | ||
| '$heap32VectorToArray', '$embind__requireFunction', | ||
| '$whenDependentTypesAreResolved', | ||
|
|
||
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.
This looks like a bad merge? Delete this line maybe?
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.
Sorry, my bad.