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

File.readAsBytes should return Uint8List #31547

Open
tvolkert opened this Issue Dec 5, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@tvolkert

tvolkert commented Dec 5, 2017

File.readAsBytes (and the sync version as well) should return Uint8List rather than List<int>. This is a backwards-compatible change since Uint8List implements List<int>.

@zanderso

@zanderso

This comment has been minimized.

Show comment
Hide comment
@zanderso

zanderso Dec 5, 2017

Member

The implementation already returns a Uint8List upcast to a List<int>.

Member

zanderso commented Dec 5, 2017

The implementation already returns a Uint8List upcast to a List<int>.

@tvolkert

This comment has been minimized.

Show comment
Hide comment
@tvolkert

tvolkert Dec 5, 2017

Awesome. Any reason not to declare that in the API?

tvolkert commented Dec 5, 2017

Awesome. Any reason not to declare that in the API?

@zanderso

This comment has been minimized.

Show comment
Hide comment
@zanderso

zanderso Dec 5, 2017

Member

I don't know. /cc @lrhn @floitschG

Member

zanderso commented Dec 5, 2017

I don't know. /cc @lrhn @floitschG

@tvolkert

This comment has been minimized.

Show comment
Hide comment
@tvolkert

tvolkert Dec 5, 2017

Here's the use case: I'm reviewing code that defines a new API for comparing binary images as bytes. The API currently takes List<int> for each image's bytes, and in my review, I stated that they should take Uint8List instead. However, if they do that, then callers will have to take the results of File.reasdAsBytes and then call new Uint8List.fromList(bytes) because they can't assume that they already have a Uint8List. They should be able to know that they're receiving a Uint8List from File.reasAsBytes

tvolkert commented Dec 5, 2017

Here's the use case: I'm reviewing code that defines a new API for comparing binary images as bytes. The API currently takes List<int> for each image's bytes, and in my review, I stated that they should take Uint8List instead. However, if they do that, then callers will have to take the results of File.reasdAsBytes and then call new Uint8List.fromList(bytes) because they can't assume that they already have a Uint8List. They should be able to know that they're receiving a Uint8List from File.reasAsBytes

@tvolkert

This comment has been minimized.

Show comment
Hide comment
@tvolkert

tvolkert Dec 5, 2017

Also, File implementations shouldn't ever be allowed to return a List<int> that's not a Uint8List.

tvolkert commented Dec 5, 2017

Also, File implementations shouldn't ever be allowed to return a List<int> that's not a Uint8List.

@matanlurey matanlurey changed the title from File.reasAsBytes should return Uint8List to File.readAsBytes should return Uint8List Dec 5, 2017

@lrhn

This comment has been minimized.

Show comment
Hide comment
@lrhn

lrhn Dec 7, 2017

Member

I would recommend changing the API to state that the byte list is a Uint8List, unless there is a good reason not to.

There are two dangers with saying List<int> and always returning Uint8List:

  1. People doing roundabout things to make it Uint8List, or
  2. People starting to depend on it being Uint8List.

In the latter case, someone might try to mock the class fail to return a Uint8List. In the former case, you get unnecessary overhead.
Or, in other words, "under-promise, over-deliver" is not a good way to design APIs :)

There is one danger with saying Uint8List: You may end up in a situation in the future where you try to implement the interface, and your bytes are not in a Uint8List, so you have to copy them then.

I don't think that last danger is an actual problem. That kind of issue only tends to crop up in testing, and performance isn't the same issue there.

Member

lrhn commented Dec 7, 2017

I would recommend changing the API to state that the byte list is a Uint8List, unless there is a good reason not to.

There are two dangers with saying List<int> and always returning Uint8List:

  1. People doing roundabout things to make it Uint8List, or
  2. People starting to depend on it being Uint8List.

In the latter case, someone might try to mock the class fail to return a Uint8List. In the former case, you get unnecessary overhead.
Or, in other words, "under-promise, over-deliver" is not a good way to design APIs :)

There is one danger with saying Uint8List: You may end up in a situation in the future where you try to implement the interface, and your bytes are not in a Uint8List, so you have to copy them then.

I don't think that last danger is an actual problem. That kind of issue only tends to crop up in testing, and performance isn't the same issue there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment