-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
stream: fix reading LibuvStream into array #56092
Conversation
This is still broken.
function mwe(filepath, nb, max_nb)
write(filepath, ones(UInt8, nb))
p = open(`cat $filepath`; read=true)
b = Vector{UInt8}(undef, 1024)
nr = readbytes!(p, b, max_nb)
return nr, b
end
nr, b = mwe(tempname(), 10, 10000)
|
I don't understand how this is intended to differ meaningfully from |
I'm confused, the "needs tests" label was removed but tests weren't added. It would be good to include a test to ensure #56078 is indeed fixed and to prevent it from regressing in the future. Since this is adding methods to an exported function, it would be good to document the methods. If they aren't intended to be user-facing, perhaps make it a separate, internal-only function? |
I wasn't sure about what it needed for a new test here, and didn't seem worth spending time on it, while the followup issues was already caught by the existing |
Why can't these just be internal functions over extending |
Would it work to use the test case from #56078? Just guessing but this might do it: function issue56078(filepath, nb, max_nb)
write(filepath, ones(UInt8, nb))
@static if Sys.iswindows()
cmd = `cmd.exe /C type $filepath`
else
cmd = `cat $filepath`
end
p = open(cmd; read=true)
b = Vector{UInt8}(undef, 1024)
nr = readbytes!(p, b, max_nb)
return nr, b
end
mktemp() do file, io
close(io)
nr, v = issue56078(file, 53209, typemax(Int))
@test nr == 53209
@test length(v) > 1024
end
We could add methods to |
I couldn't think of a better name, since they do what |
Call it |
Adds a new abstraction `take!(::Array{T,N}, ::Array{T,N})` for doing an efficient `copyto!` equivalent. Previously it was assumed that `compact` did this automatically, which wasn't a great assumption. Fixes #56078
Adds a new internal function `_take!(dst::Array{T,N}, src::Array{T,N})` for doing an efficient `copyto!` equivalent. Previously it was assumed that `compact` did this automatically, which wasn't a great assumption. Fixes #56078
Adds a new abstraction
take!(dst::Array{T,N}, src::Array{T,N})
for doing an efficientcopyto!
equivalent. Previously it was assumed thatcompact
did this automatically, which wasn't a great assumption.Fixes #56078