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 additional helpers to bitfield data structure #2876

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add additional helpers to bitfield data structure #2876

wants to merge 1 commit into from

Conversation

jbaublitz
Copy link
Contributor

Closes #2674

@jbaublitz jbaublitz force-pushed the issue-2674 branch 2 times, most recently from 01e096b to e2678d1 Compare July 30, 2024 20:07
@ojeda ojeda mentioned this pull request Jul 30, 2024
Open
21 tasks
@jbaublitz jbaublitz force-pushed the issue-2674 branch 2 times, most recently from cdda8d8 to 9099f0c Compare July 30, 2024 20:31
@ojeda ojeda added the rust-for-linux Issues relevant to the Rust for Linux project label Jul 31, 2024
@jbaublitz jbaublitz marked this pull request as draft August 1, 2024 13:56
@jbaublitz jbaublitz force-pushed the issue-2674 branch 6 times, most recently from e666f7c to 803d46b Compare August 1, 2024 14:32
@jbaublitz jbaublitz marked this pull request as ready for review August 1, 2024 14:36
@jbaublitz
Copy link
Contributor Author

@y86-dev Can you confirm that this resolves your issue? Now that it's passing CI, I think it's ready for review.

@ojeda @emilio Also feel free to let me know if you have any feedback.

@y86-dev
Copy link

y86-dev commented Aug 1, 2024 •
edited
Loading

Hi, thanks for working on this!
I tried your branch with the kernel repo and the code seems to look like what we would want, but I have a couple notes:

  1. in the kernel, we don't have std (and I would imagine that many users of bindgen would not want to rely on having it, do you test for this in your CI?) and thus the generated code doesn't compile. The fix is probably to just replace std with core.
  2. I am not so sure if my initial naming suggestion was the best, at the moment I like field_raw better than raw_field for the raw getter, the setter name seems fine to me, but if you think making it set_field_raw, then that also seems good to me.

Otherwise the code looks good to me, maybe Miguel finds something else. Also cc @nbdd0121, it might be a good idea to let him also take a look.

@jbaublitz
Copy link
Contributor Author

Hi, thanks for working on this! I tried your branch with the kernel repo and the code seems to look like what we would want, but I have a couple notes:

1. in the kernel, we don't have `std` (and I would imagine that many users of `bindgen` would not want to rely on having it, do you test for this in your CI?) and thus the generated code doesn't compile. The fix is probably to just replace `std` with `core`.

Whoops, I saw that bindgen used std in generated code, but it must be feature flagged. Regardless, I can see how that would be undesirable. I'll fix that up.

2. I am not so sure if my initial naming suggestion was the best, at the moment I like `field_raw` better than `raw_field` for the raw getter, the setter name seems fine to me, but if you think making it `set_field_raw`, then that also seems good to me.

Okay I'll change it!

Otherwise the code looks good to me, maybe Miguel finds something else. Also cc @nbdd0121, it might be a good idea to let him also take a look.

@y86-dev
Copy link

y86-dev commented Aug 2, 2024

I tested it again and it LGTM, thanks!

@jbaublitz jbaublitz changed the title Add additional helpers to bitmask data structure Add additional helpers to bitfield data structure Sep 3, 2024
@jbaublitz
Add additional helpers to bitfield data structure
170c5f9 
This commit addresses the case where a struct containing a bitfield
is wrapped in a struct such as UnsafeCell which allows interior
mutability.

Previously, bitfield accessors only allowed a receiver. This becomes
problematic in the case of interior mutability as raw pointer access may
be required so as not to violate the aliasing rules in Rust.
@jbaublitz
Copy link
Contributor Author

@pvdrz Hi! This PR is ready I think assuming @ojeda is okay with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
rust-for-linux Issues relevant to the Rust for Linux project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bitfields raw pointer accessors
3 participants
@jbaublitz @y86-dev @ojeda