[embind] Add iterable protocol support for bound classes #25993
Conversation
| const makeIterator = (size, getValue) => { | ||
| const useBigInt = typeof size === 'bigint'; | ||
| const one = useBigInt ? 1n : 1; | ||
| let index = useBigInt ? 0n : 0; |
There was a problem hiding this comment.
Could we simply dictate that interables using size_t i.e. bigint on wasm64 and number on wasm32?
I don't image we will ever see 64-bit iterators on wasm32, right?
There was a problem hiding this comment.
I tried doing this, but register_vector seems to use unsigned int on purpose, so I presume forcing Bigint on wasm64 is undesirable?
There was a problem hiding this comment.
In that case wouldn't we only need to support number here? i.e. if register_vector is using unsigned int couldn't we mandate that all iterators do the same?
There was a problem hiding this comment.
i.e. could we just assert typeof size === 'number' here, or are there cases (in the test code) where this is not true?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Fair enough, but if we want explicitly support this then maybe we should add test for it?
Also, should these lines be behind #if MEMORY64, of do you think somebody might using int64 as an interator an wasm32 ? Seems unlikely. Purring them behind #if MEMORY64 would also help signal the intent here I think.
aestuans
force-pushed
the
iterable
branch
2 times, most recently
from
2643dda to
3db19f4
Compare
|
@sbc100 Just checking in to see if this PR can get another look. Appreciate it! |
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
lgtm but I'll leave it to @brendandahl to do the final approval.
brendandahl
left a comment
brendandahl
left a comment
There was a problem hiding this comment.
Looks good to me, but please add an entry in the changelog as well.
Add `class_<T>::iterable()` to implement `Symbol.iterator`. Use this in `register_vector` so bound vectors have better ergonomics, such as working with `for...of` and `Array.from()`. This is tangentially related to emscripten-core#11070.
ChangeLog.md
Outdated
| - A new `-sEXECUTABLE` setting was added which adds a #! line to the resulting | ||
| JavaScript and makes it executable. This setting defaults to true when the | ||
| output filename has no extension, or ends in `.out` (e.g. `a.out`) (#26085) | ||
| JavaScript and makes it executable. (#26085) |
There was a problem hiding this comment.
This looks like a bad merge? Delete this line maybe?
3 participants
Add
class_<T>::iterable()to implementSymbol.iterator. Use this inregister_vectorso bound vectors have better ergonomics, such as working withfor...ofandArray.from().This is tangentially related to #11070 in that it's intended to make vector bindings nicer to use, but unlike what is suggested there it doesn't copy the vector to JS memory.