Swoorup / wgsl-bindgen

Generate typesafe Rust bindings for wgsl shaders in wgpu
https://crates.io/crates/wgsl_bindgen
MIT License
33 stars 6 forks source link

make bind group public #46

Closed EriKWDev closed 5 days ago

EriKWDev commented 1 month ago

it was marked as private

gooroo-dev[bot] commented 1 month ago

Please double check the following review of the pull request:

Issues counts

🐞Mistake 🤪Typo 🚨Security 🚀Performance 💪Best Practices 📖Readability ❓Others
0 0 0 0 1 0 0

Changes in the diff

Identified Issues

ID Type Details Severity Confidence
1 💪Best Practices Making the wgpu::BindGroup field public might expose internal details 🟠Medium 🟠Medium

Issue 1: Making the wgpu::BindGroup field public might expose internal details

Explanation

In wgsl_bindgen/src/generate/bind_group/mod.rs, making the wgpu::BindGroup field public by changing:

pub struct #bind_group_name(pub wgpu::BindGroup);

exposes internal details of the struct. This might lead to unintended usage and potential misuse of the internal wgpu::BindGroup field.

Suggested Fix

Instead of making the field public, provide a public method to access the wgpu::BindGroup field if necessary. This maintains encapsulation and allows for controlled access.

impl #bind_group_name {
    pub fn bind_group(&self) -> &wgpu::BindGroup {
        &self.0
    }
}

Explanation of the Fix

The fix introduces a public method bind_group that returns a reference to the internal wgpu::BindGroup field. This approach maintains encapsulation and provides controlled access to the internal field.

Missing Tests

To ensure the changes work as expected, add the following tests:

Test for Public Method Access

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_bind_group_access() {
        // Assuming `wgpu::BindGroup` can be instantiated for testing
        let bind_group = wgpu::BindGroup::new(); // Replace with actual instantiation
        let my_bind_group = #bind_group_name(bind_group);

        // Test the public method
        assert_eq!(my_bind_group.bind_group(), &bind_group);
    }
}

This test checks if the public method bind_group correctly returns a reference to the internal wgpu::BindGroup field. Adjust the instantiation of wgpu::BindGroup as necessary for your testing environment.

Summon me to re-review when updated! Yours, Gooroo.dev Feel free to add a reaction or reply with your feedback!

EriKWDev commented 1 month ago

you do you bot

Swoorup commented 1 month ago

Can you also run all the tests and update the expected code snapshot if necessary?