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.

Sign up for GitHub

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

Jump to bottom

Add remove_front and remove_back method #253

Merged
merged 55 commits into from
Jul 17, 2023
Merged

Add remove_front and remove_back method #253

merged 55 commits into from
Jul 17, 2023

Conversation

shimatar0
Copy link
Contributor

@shimatar0 shimatar0 commented May 12, 2023 •
edited
Loading

What does this PR do?

  • added remove the first n elements method and remove the last n elements method

shimatar0 and others added 26 commits May 7, 2023 12:40
@shimatar0
Feat remove first
dcc5eeb
@shimatar0
Fix process
2009f6e
@shimatar0
Fix remove first function
389da10
@shimatar0
Fix bit switching process of remove_first
758bb6d
@shimatar0
Feat remove last
db3df9e
@shimatar0
Fix container remove target
e8e9be6
@shimatar0
Fix where counts are subtracted and add test
6a3ffb5
@shimatar0
Add handle of cases where the target to be deleted is larger than the…
43a88e9 
… length of the container
@shimatar0
Fix to use rposition for remove_last
e92df34
@shimatar0
Add test
85a8f02
@shimatar0
Add test to delete 0 elements
ede6cde
@shimatar0
Remove unnecessary if statement
33b540c
@shimatar0
Add all bit clear
f7601db
@shimatar0
Change conditional expression
1cd29e3
@shimatar0
Remove hard to follow variables
7f02d44
@shimatar0
Change the part that loops unnecessary
ad19557
@Kerollmops
Bump Rust version to 1.65.0
acf4ecd
@Kerollmops
Update git2 for security reasons
b9b3246
@Kerollmops
Change the workflow branche from master to main
203012f
@Kerollmops
Fix clippy
81f1f2a
@Kerollmops
Remove bors from the repo
9f0d4d0
@Kerollmops
Merge pull request RoaringBitmap#256 from RoaringBitmap/bump-rust-ver…
25251c9 
…sion-to-1.60.0

Bump Rust version to 1.65 and remove Bors
@Kerollmops @josephglanville
Deserialize run containers and convert them into bitmap/array
5618314 
Co-authored-by: Joseph Glanville <jpg@jpg.id.au>
@Kerollmops
Add a test for the run containers
8f23ccf
@Kerollmops
Update README badge
95664bd
@Kerollmops
Merge pull request RoaringBitmap#255 from RoaringBitmap/deserialize-r…
6aabe69 
…un-containers

Deserialize Run Containers
Kerollmops
Kerollmops requested changes Jul 10, 2023
View reviewed changes
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Hey @shimatar0 👋, Could you please rebase on main?

Kerollmops
Kerollmops requested changes Jul 10, 2023
View reviewed changes
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

About both RoaringBitmap::remove_first/last methods implementations. Would it be possible not to check the length before, as I am pretty sure it takes time to compute it? It is O(n) where n is the number of containers.

Can we find better names? I would prefer remove_front and remove_back to match the pop_front/back API of the VecDeque collection. What do you think?

Would it be possible to change the API of those too methods to accept u64 instead of usize? It would be more consistent with the rest of the API and will work correctly on u32 computers.

src/bitmap/inherent.rs Outdated Show resolved Hide resolved
src/bitmap/inherent.rs Outdated Show resolved Hide resolved
src/bitmap/inherent.rs Outdated Show resolved Hide resolved
src/bitmap/inherent.rs Outdated Show resolved Hide resolved
src/bitmap/inherent.rs Outdated Show resolved Hide resolved
src/bitmap/store/array_store/mod.rs Outdated Show resolved Hide resolved
src/bitmap/store/array_store/mod.rs Show resolved Hide resolved
Dr-Emann
Dr-Emann reviewed Jul 10, 2023
View reviewed changes
src/bitmap/store/bitmap_store.rs Outdated Show resolved Hide resolved
src/bitmap/store/bitmap_store.rs Outdated Show resolved Hide resolved
src/bitmap/container.rs Outdated Show resolved Hide resolved
src/bitmap/container.rs Outdated Show resolved Hide resolved
src/bitmap/inherent.rs Outdated Show resolved Hide resolved
src/bitmap/inherent.rs Outdated Show resolved Hide resolved
@Kerollmops
Copy link
Member

Hey @shimatar0 👋,

Could you please apply the suggestions of @Dr-Emann. We will look into rebasing your branch afterward. Unfortunately, the CI doesn't pass. Could you fix it too? Note: That's due to Clippy.

@shimatar0
Copy link
Contributor Author

@Kerollmops

About both RoaringBitmap::remove_first/last methods implementations. Would it be possible not to check the length before, as I am pretty sure it takes time to compute it? It is O(n) where n is the number of containers.

@Dr-Emann's suggestion eliminates the need to know the length in advance:)
6b54277

Can we find better names? I would prefer remove_front and remove_back to match the pop_front/back API of the VecDeque collection. What do you think?

Renamed methods.
f4080d9

Would it be possible to change the API of those too methods to accept u64 instead of usize? It would be more consistent with the rest of the API and will work correctly on u32 computers.

Use u64 instead of `usize.
b7c5b5a

@shimatar0
Copy link
Contributor Author

Hey! @Kerollmops 👋

Thanks for the your review.
Fixed the suggestion so that there is no clippy warning.

Could you please apply the suggestions of @Dr-Emann. We will look into rebasing your branch afterward. Unfortunately, the CI doesn't pass. Could you fix it too? Note: That's due to Clippy.

@Kerollmops Kerollmops changed the title Add remove_first and remove_last method Add remove_front and remove_back method Jul 14, 2023
@Kerollmops
Copy link
Member

Kerollmops commented Jul 15, 2023 •
edited
Loading

Hey @Dr-Emann 👋

I wanted to thank you very much for this particularly good code review of yours! Always impressed by what you spot ☺️

Have a nice weekend ☀️

@Kerollmops Kerollmops merged commit 9f69285 into RoaringBitmap:main Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dr-Emann Dr-Emann Dr-Emann left review comments

@Kerollmops Kerollmops Kerollmops approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shimatar0 @Kerollmops @Dr-Emann