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

Added atomic bitflags generation #391

Closed
wants to merge 4 commits into from
Closed

Conversation

f-gagel
Copy link

@f-gagel f-gagel commented Jan 7, 2024

The idea is to enable the macro to create a wrapper type for atomic flags, using an attribute as opt in. All the main flags logic will still be handled by the regular flags, with the atomic flags only acting as an adapter to some atomic int type.

bitflags! {
    #[atomic_attr(derive(Default))]
    #[atomic(MyAtomicFlags)]
    pub struct MyFlags: u32 {
        const A = 1;
        const B = 2;
    }
}

fn main() {
    let flags = MyAtomicFlags::default();
    flags.fetch_insert(MyFlags::A, Ordering::Relaxed);
}

Some open questions to me are:

  • Is the attribute sufficient as an opt in or should a feature flag be added?
  • Is there a nicer way to declare the atomic flags type and especially its attributes?

@KodrAus
Copy link
Member

KodrAus commented Jan 16, 2024

Hi @f-gagel 👋 Thanks for the PR! I've tried to avoid introducing any custom attributes to bitflags to keep the implementation from becoming too complex.

I had a look at whether we could already do this with a custom Bits implementation and we can't really, because you'll want to operate on non-atomic integers, but store the results atomically.

Depending on your use-case, you could declare your bitflags type over a regular integer type, and just load/store it from a shared atomic integer as necessary. That would neatly separate storage concerns from the flags type itself, and let you do multiple bitwise operations on the flags value before needing to use atomics to store them.

@f-gagel
Copy link
Author

f-gagel commented Feb 3, 2024

Hi. Thanks for the feedback!

I understand the concern about keeping the macro straightforward. For the atomic storage would a generic wrapper type Atomic<Flags> be an option? My goal with this is to not just enable load and store but also fetch_* operations for flags or would that be outside the scope of the bitflags crate?

@kkysen
Copy link

kkysen commented Mar 4, 2024

Hi @f-gagel, I've been able to accomplish something similar with atomig by #[derive(Atom, AtomLogic)]ing. It doesn't result in exactly the same API as this PR, but it's pretty similar. You can do Atomic<Flags> and things like .fetch_or.

@KodrAus
Copy link
Member

KodrAus commented Mar 20, 2024

Thanks @kkysen 👍 That seems like a good way to go here.

I think this isn't something we'll be able to support in bitflags directly due to the additional feature complexity so will go ahead and close this one, but I appreciate the time you've spent putting an implementation together.

@KodrAus KodrAus closed this Mar 20, 2024
@f-gagel
Copy link
Author

f-gagel commented Apr 3, 2024

Hi, I understand that you want to avoid that extra complexity. Thank you for your consideration.
@kkysen atomig works really nicely for this case. Thanks for pointing it out.

To anyone looking for how to get my preferred API, here's the neccessary glue: https://gist.github.com/f-gagel/7cb44e6ce27022cf2991b11d27044610

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.

None yet

3 participants