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 from_bits_[ref|mut] for FFI in place ops #184

Closed
wants to merge 3 commits into from

Conversation

flier
Copy link
Contributor

@flier flier commented Jul 1, 2019

When use FFI struct, we always need to copy the field to a flags and write it back after change it

#[repr(C)]
struct Foo {
   bar: u32,
}

let mut flags = Flags::from_bits_truncate(foo.bar);
// ...
flags |= Flags::SOME;
// ...
foo.bar = flags.bits();

With from_bits_ref and from_bits_mut method, we could support in place operations, like

let flags = Flags::from_bits_mut(&mut foo.bar);
// ...
*flags |= Flags::SOME;

Every change to the underlying field could be read immediately,
and the mutable reference can be pass/return to other function, no more write back.

@KodrAus
Copy link
Member

KodrAus commented Jul 16, 2019

Thanks for the PR @flier!

I'm a little concerned about introducing unsafe into the macros, because it will break folks compiling with #[deny(unsafe)], and leave no option for folks compiling with #[forbid(unsafe)].

The #[repr(transparent)] change looks like a good one though.

@flier
Copy link
Contributor Author

flier commented Jul 23, 2019

I think we may introduce an unsafe feature for those scenes which need work with unsafe FFI code, and disable it by default for compatibility. @KodrAus

@KodrAus
Copy link
Member

KodrAus commented Jul 25, 2019

@flier Hmm, it would be unfortunate to leak the fact that this requires unsafe to implement that way. It's not a discoverable way to flag it. So I think our best path forward in bitflags is to add just the #[repr(transparent)] attribute so it's valid for you to write the cast in your own FFI code.

We could add a section under our crate docs called Layout or FFI, and note that your bitflags has the same size and alignment of T, but not all valid bit patterns of T are guaranteed to be valid flags.

@flier flier mentioned this pull request Jul 26, 2019
@flier
Copy link
Contributor Author

flier commented Jul 26, 2019

Fair, I have submit another PR #187 to add the #[repr(transparent)] attribute, thank anyway

@flier flier closed this Jul 26, 2019
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

2 participants