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.

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

Merged
merged 4 commits into from
Jun 24, 2024

Conversation

nhz2
Copy link
Member

@nhz2 nhz2 commented May 30, 2024

Fixes #211

const ByteData = Union{Vector{UInt8},Base.CodeUnits{UInt8}}

This may cause issues as CodeUnits may not be stored densely in memory.
Ref: JuliaLang/julia#54002

@nhz2 nhz2 marked this pull request as ready for review May 30, 2024 18:07
@nhz2 nhz2 requested review from jakobnissen and mkitti May 30, 2024 18:07
@mkitti
Copy link
Member

mkitti commented May 30, 2024

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

@nhz2
Copy link
Member Author

nhz2 commented May 30, 2024

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?

@nhz2 nhz2 changed the title Remove buggy Memory(::ByteData) constructor. Remove buggy Memory(::Base.CodeUnits{UInt8}) constructor. May 30, 2024
@jakobnissen
Copy link
Collaborator

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.

Copy link
Collaborator

@jakobnissen jakobnissen left a 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.

@mkitti
Copy link
Member

mkitti commented May 31, 2024

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

@nhz2
Copy link
Member Author

nhz2 commented May 31, 2024

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: JuliaIO/CodecLz4.jl#47

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

@mkitti
Copy link
Member

mkitti commented Jun 1, 2024

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.

@nhz2 nhz2 marked this pull request as draft June 23, 2024 14:44
@nhz2 nhz2 changed the title Remove buggy Memory(::Base.CodeUnits{UInt8}) constructor. Remove Memory(::ByteData) constructor Jun 23, 2024
@nhz2 nhz2 marked this pull request as ready for review June 24, 2024 01:28
@nhz2 nhz2 requested a review from jakobnissen June 24, 2024 01:28
@nhz2 nhz2 dismissed jakobnissen’s stale review June 24, 2024 16:39

I'm deprecating for now to avoid compatibility issues. I'll remove completely in the next breaking release.

@nhz2 nhz2 merged commit 2e7f5fc into master Jun 24, 2024
26 checks passed
@nhz2 nhz2 deleted the nz/Remove-buggy-memory-constructor branch June 24, 2024 16:40
nhz2 added a commit that referenced this pull request Jun 24, 2024
This release includes several bug fixes and the deprecation of the `Memory(::ByteData)` constructor (#219)
@nhz2 nhz2 mentioned this pull request Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ByteData may not be densely stored in memory
3 participants