search

JuliaIO / TranscodingStreams.jl

Simple, consistent interfaces for any codec.
https://juliaio.github.io/TranscodingStreams.jl/
Other
86 stars 28 forks source link

Remove `Memory(::ByteData)` constructor #219

Closed nhz2 closed 4 months ago

nhz2 commented 5 months ago

Fixes #211

https://github.com/JuliaIO/TranscodingStreams.jl/blob/8fdabeda88199cd4298d9cba597e006b1af7655a/src/TranscodingStreams.jl#L8

This may cause issues as CodeUnits may not be stored densely in memory. Ref: https://github.com/JuliaLang/julia/pull/54002

mkitti commented 5 months ago

This looks like a breaking change. Could we deprecate the use of ByteData before we remove it?

nhz2 commented 5 months ago

This is a bug fix. It isn't possible to turn a general CodeUnits into a Memory. Is there any not already buggy code this would break?

jakobnissen commented 5 months ago

The constructor is unused in the codebase, unexported, and explicitly mentioned to be internal in the documentation. I would rather see that constructor completely removed.

mkitti commented 5 months ago

I agree with the change. Possibly agree with removing the constructor. How is Memory usually constructed?

nhz2 commented 5 months ago

To remove the Memory(::Vector{UInt8}) constructor, https://github.com/JuliaIO/CodecLz4.jl/blob/55c2c423f8b757f2325dc83dd5473107375f823c/test/frame_compression.jl#L55-L56 would need to be updated. Though I agree that this should be removed, it already caused some issues, Ref: https://github.com/JuliaIO/CodecLz4.jl/pull/47

Memory is usually constructed with Memory(ptr::Ptr{UInt8}, size::UInt)

mkitti commented 5 months ago

While I agree with @jakobnissen that there is a more fundamental issue, I think this pull request can go through to advance things in the right direction. I suspect that we will need a more fundamental refactoring here to eliminate free pointers hanging around.