Skip to content

Initial (optional) triomphe::Arc support#454

Open
virtualritz wants to merge 2 commits into
Amanieu:masterfrom
virtualritz:master
Open

Initial (optional) triomphe::Arc support#454
virtualritz wants to merge 2 commits into
Amanieu:masterfrom
virtualritz:master

Conversation

@virtualritz

Copy link
Copy Markdown

See subject.

This adds support for swapping out std::sync::Arc (parking_lot) resp. alloc::sync::Arc (lock_api) for triomphe::Arc.

Both crates had a triomphe feature added but alternatively I could put this behind a triomphe_arc_lock feature instead.
Or even arc_de_triomphe_lock, to fully express the presumed naming intent of that crate. 😉

I need this for a project I'm working on so I thought I may as well open a PR, in case this is useful. If not, just close the PR and ignore.

@virtualritz virtualritz marked this pull request as ready for review February 4, 2025 12:16

@Amanieu Amanieu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Sorry for the late response, this slipped off my review queue.

Overall, I'm very much in favor of adding triomphe support, but the issue with the way the code is written is that it doesn't allow both std::sync::Arc and triomphe::Arc to be supported at the same time. Instead, I would prefer if methods like arc_lock were generic over a trait which is implemented for both std::sync::Arc and triomphe::Arc (and possibly other Arc variants from triomphe).

Comment thread src/condvar.rs
mod tests {
use crate::{Condvar, Mutex, MutexGuard};
use std::sync::mpsc::channel;
#[cfg(not(feature = "triomphe"))]

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

There's no need to do this for tests, we can always use std::sync::Arc there.

@virtualritz

virtualritz commented May 23, 2025

Copy link
Copy Markdown
Author

[...] Instead, I would prefer if methods like arc_lock were generic over a trait which is implemented for both std::sync::Arc and triomphe::Arc (and possibly other Arc variants from triomphe).

That makes a lot of sense. But it does sound like quite a bit of work unless I miss something?

I am afraid it will be a long time before I can work on this -- if ever.

This PR came to be because we use a forked parking_lot and triomphe::Arc everywhere. But we don't have the requirement of using it alongside std::sync::Arc or mixing those two. And as a startup we don't have the resources to add features outside our own requirements to 3rd party crates -- I'm one of two Rust devs ... heaps sorry. 😢

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.

2 participants