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

skiplist: add compare_insert #976

Merged
merged 7 commits into from
Aug 26, 2023
Merged

skiplist: add compare_insert #976

merged 7 commits into from
Aug 26, 2023

Conversation

Tianion
Copy link
Contributor

@Tianion Tianion commented Apr 13, 2023

The purpose is support compare and swap / compare and exchange.
I notice that the function insert_internal is core function, so I change replace to support CAS

@Tianion
Copy link
Contributor Author

Tianion commented Apr 18, 2023

hhh...CI / san (pull_request) Cancelled after 360m. There seems no problem with this commit
example

let map = SkipMap::new();
map.insert("key", 1);
map.compare_insert("key", 0, |x| x < &0);
assert_eq!(*map.get("key").unwrap().value(), 1);
map.compare_insert("key", 2, |x| x < &2);
assert_eq!(*map.get("key").unwrap().value(), 2);

@taiki-e
Copy link
Member

taiki-e commented Aug 23, 2023

Thanks for the PR! CI failure should be fixed by rebasing.

@taiki-e
Copy link
Member

taiki-e commented Aug 23, 2023

Could you also add a test for a case where the key is not present? (should work the same as insert)

Also, it would be nice to clarify in the documentation that the closure will not be called if the key is not present.

@Tianion
Copy link
Contributor Author

Tianion commented Aug 26, 2023

The rustc version in the CI has been upgraded from 1.71 to 1.72, which resulted in CI/Clippy checks failing. I attempted to fix it, but there are certain issues below that I'm unsure how to address.
As a workaround, I had to use #[allow(clippy::arc_with_non_send_sync)] to pass the CI/clippy. I tried replacing Arc with Rc, but this would likely introduce other problems.

error: usage of an `Arc` that is not `Send` or `Sync`
  --> crossbeam-epoch/src/collector.rs:32:21
   |
32 |             global: Arc::new(Global::new()),
   |                     ^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: the trait `Sync` is not implemented for `Global`
   = note: required for `Arc<Global>` to implement `Send` and `Sync`
   = help: consider using an `Rc` instead or wrapping the inner type with a `Mutex`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync
   = note: `-D clippy::arc-with-non-send-sync` implied by `-D warnings`

@taiki-e
Copy link
Member

taiki-e commented Aug 26, 2023

Could you remove (or revert) clippy-related commits? They have been addressed in #1021.

@Tianion
Copy link
Contributor Author

Tianion commented Aug 26, 2023

Could you remove (or revert) clippy-related commits? They have been addressed in #1021.

Done,thanks for your detailed review

Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks!

@taiki-e taiki-e merged commit 0720460 into crossbeam-rs:master Aug 26, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants