-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
dart: Return dart:typed_data equivalent where possible #8289
base: master
Are you sure you want to change the base?
Conversation
|
I'm not opposed to that, but I'd like to see some insight from one of the Dart maintainers to see if this aligns with their view for the Dart side. Ideally, we could expand the same to |
Ready for review. I tested using @tompark's example code, with the latest version of flatbuffers on pub.dev as well as this fork, and this fork prints the expected |
* Remove _FbUint8List, _FbInt8List and _FbUint16List to return typed_data types * Add test for Int8List * Test Int8List, Uint8List and Uint16List for type equivalence * Remove unnecessary cast in _writeUTFString to silence linting warning
I squashed the changes and removed a change related to my other PR to prevent conflicts between the two. @vaind Hi Ivan, is there any way you might be able to spare some bandwidth in the relatively near future to help review and potentially land both of my open PRs? Cheers. |
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.
Thanks a lot. It looks pretty good to me, although there need to be some changes to make the code correct on Big endian machines (I know these are basically non-existent, but since Dart supports them, we should too).
I think I'd rather add a separate path or built-in for big-endian, otherwise the newly added conditions to the tests will cause the tests to fail on big-endian, as the old classes do not return Uint8List, Int8List, Uint16List, etc on the lazy path. I think the only supported test environment (theoretically) would be ARM on Linux running in big-endian mode? I'll try and set up a qemu environment to test. |
I think it's OK to have different actual return type on big endian, the only contract is that it is compatible with List, everything else is "just" an optimization. It's OK for tests to assert the return value is the expected type based on endianness though |
Sounds good to me. Sorry for the delay. I'll get this updated this week, hopefully on the earlier half as opposed to the later half. |
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.
LGTM.
Are you going to do the other lists (int32, ...) too or is this PR finished?
I'll extend this to other types! I'll expand the tests as well. |
For this PR specifically, it has been expanded out to the existing List types in the code already. I don't know enough about the flatbuffers library to understand how to expand this out for types that do not already have a Reader class, eg Int16ListReader, Int32ListReader. Note though, the float implementation needs to be corrected, but after that the PR should be complete. |
@vaind I couldn't find out what is causing Float64List to fail, so I am going to revert the changes to the Float types and mark them with a todo to return their dart:typed_data equivalent so the PR can be restored to a working state and will be ready for review. |
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.
LGTM but I cannot merge as I'm not a maintainer.
@dbaileychess Ping. Let me know if anything should be changed. |
Stems from discussion in #8183. This should allow users to be able to access a Uint8List directly without copying over from a list, thus actually offering zero-copy access (for this class, at least) in both the lazy path and the non-lazy path. This is pretty important for signal processing and image processing where we want to avoid any unnecessary list copies.