Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Widen signature of read!(::IO, ::Array) to AbstractArray #33046

Merged
merged 1 commit into from
Dec 10, 2019
Merged

Widen signature of read!(::IO, ::Array) to AbstractArray #33046

merged 1 commit into from
Dec 10, 2019

Conversation

giordano
Copy link
Contributor

Fix #33035.

yuyichao
yuyichao requested changes Aug 23, 2019
View reviewed changes
Copy link
Contributor

@yuyichao yuyichao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This either have to support non-dense view with a separate implementation or explicity check for that. The isbitstype implementation definitely doesn't work with that.

Also need to add test for this case.

mbauman
mbauman reviewed Aug 23, 2019
View reviewed changes
@mbauman mbauman added the needs tests Unit tests are required for this change label Aug 23, 2019
@giordano giordano mentioned this pull request Sep 9, 2019
Merged
@tlnagy tlnagy mentioned this pull request Sep 13, 2019
Closed
@giordano
Copy link
Contributor Author

After restricting the signature as suggested by Matt, is there anything I need to do here? It's not really clear to me whether I need to test some other cases now

@giordano giordano mentioned this pull request Sep 13, 2019
Closed
@StefanKarpinski
Copy link
Member

Bump.

@giordano
Copy link
Contributor Author

Bump 🙂

mbauman
mbauman reviewed Oct 29, 2019
View reviewed changes
@giordano
Copy link
Contributor Author

I couldn't come up with a test for a non-isbits array, especially since we can't write non-isbits arrays to a buffer 🤔 Any suggestion for other tests is welcome

@giordano @mbauman
Widen signature of read!(::IO, ::Array) to AbstractArray
a12b49c 
Co-Authored-By: Matt Bauman <mbauman@gmail.com>
@giordano giordano changed the title Loosen signature of read!(::IO, ::Array) to accept a view Widen signature of read!(::IO, ::Array) to AbstractArray Nov 25, 2019
@StefanKarpinski StefanKarpinski removed the needs tests Unit tests are required for this change label Dec 3, 2019
@StefanKarpinski
Copy link
Member

Looks good to go. @mbauman, if you could give it a once over and merge that would be great.

@giordano
Copy link
Contributor Author

Bump 🙂 It would be nice to have this merged by the feature freeze for Julia 1.4

@StefanKarpinski StefanKarpinski merged commit 1dbccbd into JuliaLang:master Dec 10, 2019
@giordano giordano deleted the read!-view branch December 10, 2019 19:47
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
@giordano @mbauman @KristofferC
Widen signature of read!(::IO, ::Array) to AbstractArray (#33046)
33aca69 
Co-Authored-By: Matt Bauman <mbauman@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbauman mbauman mbauman approved these changes

@yuyichao yuyichao yuyichao requested changes

Assignees

@mbauman mbauman

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Loosen signature of read! to accept views?
4 participants
@giordano @StefanKarpinski @mbauman @yuyichao