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

add insert! #75

Merged
merged 7 commits into from
Nov 20, 2021
Merged

add insert! #75

merged 7 commits into from
Nov 20, 2021

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Nov 16, 2021

Fixes #74

@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #75 (f80d58e) into main (f87b540) will increase coverage by 0.36%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #75      +/-   ##
==========================================
+ Coverage   86.64%   87.00%   +0.36%     
==========================================
  Files           1        1              
  Lines         322      331       +9     
==========================================
+ Hits          279      288       +9     
  Misses         43       43              
Impacted Files Coverage Δ
src/PooledArrays.jl 87.00% <100.00%> (+0.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f87b540...f80d58e. Read the comment docs.

Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

LGTM; we don't need to worry about the copy-on-write business at all here or in push!, right?

@bkamins
Copy link
Member Author

bkamins commented Nov 18, 2021

Copy-on-write is handled here https://github.com/JuliaData/PooledArrays.jl/blob/main/src/PooledArrays.jl#L567, which gets called if a new entry needs to be added.

However push! and insert! are not thread safe - should me mention this in the docs or this is obvious?

@quinnj
Copy link
Member

quinnj commented Nov 18, 2021

Ah, that's right; I missed that it gets handled from the getpoolidx call. Yeah, I think it'd be worth mentioning that they aren't threadsafe; it would hopefully be obvious to most users, since they aren't threadsafe for Base Array, but it doesn't hurt as a reminder.

src/PooledArrays.jl Outdated Show resolved Hide resolved
src/PooledArrays.jl Outdated Show resolved Hide resolved
@bkamins
Copy link
Member Author

bkamins commented Nov 18, 2021

OK - I have added comments (I think it does not make much sense to add a docstring as it would be the same as for general push! and insert!)

@bkamins bkamins merged commit 53f57d4 into main Nov 20, 2021
@bkamins bkamins deleted the bkamins-patch-2 branch November 20, 2021 20:07
@bkamins
Copy link
Member Author

bkamins commented Nov 20, 2021

Thank you!

I will make a release.

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.

add insert!
3 participants