-
Notifications
You must be signed in to change notification settings - Fork 28
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
Remove Memory(::ByteData)
constructor
#219
Conversation
This looks like a breaking change. Could we deprecate the use of |
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? |
Memory(::ByteData)
constructor.Memory(::Base.CodeUnits{UInt8})
constructor.
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. |
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.
IMO, remove this constructor completely, also from the docs. Make the user pass in a pointer themselves if they need this type.
I agree with the change. Possibly agree with removing the constructor. How is |
To remove the
|
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. |
Memory(::Base.CodeUnits{UInt8})
constructor.Memory(::ByteData)
constructor
I'm deprecating for now to avoid compatibility issues. I'll remove completely in the next breaking release.
This release includes several bug fixes and the deprecation of the `Memory(::ByteData)` constructor (#219)
Fixes #211
TranscodingStreams.jl/src/TranscodingStreams.jl
Line 8 in 8fdabed
This may cause issues as
CodeUnits
may not be stored densely in memory.Ref: JuliaLang/julia#54002