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 a callback to rename field names #2524

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

expenses
Copy link

@expenses expenses commented May 13, 2023

I've been using this callback to rename anonymous fields:

#[derive(Debug)]
struct BindgenCallbacks {
    item_replacements: HashMap<&'static str, &'static str>,
    field_name_replacements: HashMap<(&'static str, &'static str), &'static str>,
}

impl BindgenCallbacks {
    fn new() -> Self {
        let mut item_replacements = HashMap::new();
        item_replacements.insert("ResourceDescPacked__bindgen_ty_1", "ResourceBufferImageDescPacked");
        item_replacements.insert("ResourceDescPacked__bindgen_ty_1__bindgen_ty_1", "ResourceImageDescPacked");
        item_replacements.insert("ResourceDescPacked__bindgen_ty_1__bindgen_ty_2", "ResourceBufferDescPacked");
        item_replacements.insert("ResourceDescPacked__bindgen_ty_1__bindgen_ty_1__bindgen_ty_1", "ResourceImageDepthArrayLayersDescPacked");
    
        let mut field_name_replacements = HashMap::new();
    
        field_name_replacements.insert(("ResourceDescPacked", "__bindgen_anon_1"), "buffer_image");
        field_name_replacements.insert(("ResourceImageDescPacked", "__bindgen_anon_1"), "depth_array_layers");

        Self {
            item_replacements, field_name_replacements
        }
    }
}

impl bindgen::callbacks::ParseCallbacks for BindgenCallbacks {
    fn process_field_name(&self, parent_name: &str, name: &str) -> Option<String> {
        if let Some(&replacement) = self.field_name_replacements.get(&(parent_name, name)) {
            Some(String::from(replacement))
        } else {
            None
        }
    }
    
    ...
}

Source struct (from https://github.com/GPUOpen-LibrariesAndSDKs/RenderPipelineShaders):

struct ResourceDescPacked
    {
        RpsResourceType  type : 8;            ///< An enumeration indicating the type (and dimension) of the resource.
        uint32_t         temporalLayers : 8;  ///< The number of frames of temporal data.
        RpsResourceFlags flags : 16;          ///< A collection of <c><i>RpsResourceFlagBits</i></c> values.

        union
        {
            struct
            {
                uint32_t width;   ///< The width of an image, or low 32 bit of the byte size of a buffer.
                uint32_t height;  ///< The height of an image, or high 32 bit of the byte size of a buffer.
                union
                {
                    uint32_t depth;        ///< The depth of an 3D image.
                    uint32_t arrayLayers;  ///< The number of array layers for an non-3D image.
                };
                uint32_t  mipLevels : 8;    ///< The number of mipmap levels.
                RpsFormat format : 8;       ///< A platform independent format to be interepreted by the runtime.
                uint32_t  sampleCount : 8;  ///< The number of MSAA samples of an image.
            } image;
            struct
            {
                uint32_t sizeInBytesLo;
                uint32_t sizeInBytesHi;
            } buffer;
        };
}

I'm not sure whether this is implemented in the best way. Very happy to make changes.

@bors-servo
Copy link

☔ The latest upstream changes (presumably 4faa366) made this pull request unmergeable. Please resolve the merge conflicts.

@@ -135,6 +135,15 @@ pub trait ParseCallbacks: fmt::Debug {
fn process_comment(&self, _comment: &str) -> Option<String> {
None
}

/// Allows renaming the name of a field, replacing `_name`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you specify what parent_name is? Assuming it's the struct type, something like type_name may be better, since usually parent/child refer to nested types or modules (rather than fields).

let name = ctx.rust_mangle(name);
ctx.options()
.process_field_name(&parent_name, &name)
.map(|name| Cow::Owned(name))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.map(|name| Cow::Owned(name))
.map(Cow::Owned)

.map(|name| {
let name = ctx.rust_mangle(name);
ctx.options()
.process_field_name(&parent_name, &name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Clippy says

this expression creates a reference which is immediately dereferenced by the compiler

can you remove the &? (not sure which one or both)

ctx.options()
.process_field_name(&parent_name, &name)
.map(|name| Cow::Owned(name))
.unwrap_or(name.to_owned())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.unwrap_or(name.to_owned())
.unwrap_or(name.into_owned())

Per clippy

this to_owned call clones the Cow<', str> itself and does not cause the Cow<', str> contents to become owned

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