Allow direct access to unnamed union fields on perf_event_attr#48
Merged
Phantomical merged 6 commits intojimblandy:masterfrom May 7, 2025
Merged
Conversation
…ndy#46) This means that future changes made to the C perf_event_attr struct which replace existing fields with inline unions can continue to be non-breaking changes to the crate. Furthermore, it is generally much more convenient to directly access the relevant fields instead of needing to go through the __bindgen_anon_x field first.
std::mem::offset_of! has been stable for long enough that memoffset is no longer necessary.
Owner
|
I haven't looked at the implementation here, but overall I'm less concerned about this change than #49, since it doesn't seem to restrict what people can do. I'll try to take a look at the code tomorrow. |
Closed
Collaborator
Author
|
I'm going to go ahead and merge this as it seems to be uncontroversial and it has been a month. I would like to move forward with changes to perf-event itself and this change is blocking that. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I've pulled this out of #42. It allows users to directly access fields in the various unnamed unions within
perf_event_attrdirectly, like they would in C. This has two main advantages:attr.bp_lenrather thanattr.__bindgen_anon_1.bp_len.#[non_exhaustive], it makes pretty much all binding updates non-breaking changes.The real benefit is 1, since it makes a whole bunch of code a little bit nicer. 2 is quite nice as well, but I think this is worth doing just for 1. The downside to this is that it's a giant hack, and requires some care to maintain when updates happen. I have included tests and static checks to verify that things work as expected, and in practice I haven't found it to be too much of a challenge keeping things up to date. IMO the upsides to this for users of the crate have been somewhat cursed code that is required to implement it.
I'd like to either get this merged or NACKed before I go upstreaming changes from perf-event2 itself.