Skip to content

Empty head block's data when the block is reset#888

Merged
eulerkochy merged 2 commits intoJuliaCollections:masterfrom
eulerkochy:fix_deque
Jan 5, 2024
Merged

Empty head block's data when the block is reset#888
eulerkochy merged 2 commits intoJuliaCollections:masterfrom
eulerkochy:fix_deque

Conversation

@eulerkochy
Copy link
Copy Markdown
Member

Fixes #884

Comment thread src/deque.jl Outdated
Comment on lines +41 to +42
empty!(blk.data)
blk.data = Vector{T}(undef, blk.capa)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why empty it if its just going to be over written with a newly allocated array?
I would think just:

Suggested change
empty!(blk.data)
blk.data = Vector{T}(undef, blk.capa)
empty!(blk.data)

would be enough.
(and it's nonallocating)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The underlying the assumption of a block is that it's data field will have already data field allocated with capa number of elements. Just emptying and not allocating block's data leads to the problem that once you have cleared the deque, you can't push elements into it again. That's why I've added new testset push! after empty! to check this scenario.

Copy link
Copy Markdown
Member

@oxinabox oxinabox Jan 2, 2024

Choose a reason for hiding this comment

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

So we want:

Suggested change
empty!(blk.data)
blk.data = Vector{T}(undef, blk.capa)
# Fill it with undefs/junk data back to capacity
empty!(blk.data); resize!(blk.data, blk.capa)

?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes. made this change

@eulerkochy eulerkochy merged commit 83ecd27 into JuliaCollections:master Jan 5, 2024
kpamnany pushed a commit to kpamnany/DataStructures.jl that referenced this pull request Feb 16, 2024
Empty head block's data when the block is reset
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.

Deque leaks memory

2 participants