gfx-rs / metal-rs

Rust bindings for Metal
Apache License 2.0
567 stars 112 forks source link

fix: Address most Clippy warnings #311

Closed tim-harding closed 3 months ago

tim-harding commented 4 months ago

These categories of warnings were addressed:

There are still some warnings about documenting unsafe functions and returning references in new functions, but I think a separate pull request will be better for those since they probably require more discussion.

MarijnS95 commented 4 months ago

Should the CI enforce these going forward? Otherwise you'll see such work being nullified sooner than later.

waywardmonkeys commented 4 months ago

This is pretty likely to conflict with my changes in #310. Not sure which will be more annoying to rebase atop the other.

tim-harding commented 4 months ago

@MarijnS95 Probably not yet. There are still some outstanding warnings that I mentioned in the post, so CI would not pass anything right now if Clippy warnings were denied. I don't mind a little whack-a-mole until we get to that point.

@waywardmonkeys I'm sure this refactor is distinctly lower-priority than yours, so I'd expect to be the one rebasing :)

Maybe this is the wrong place for it, but I have a couple of related questions. With functions like this

impl ArgumentDescriptor {
    pub fn new<'a>() -> &'a ArgumentDescriptorRef {
        unsafe {
            let class = class!(MTLArgumentDescriptor);
            msg_send![class, argumentDescriptor]
        }
    }
}

For the ones like the above that are giving warnings, shouldn't they return owned types? In the example above, the docs say that argumentDescriptor creates a new value, which I would assume causes a memory leak when the reference is dropped. The "you own any object you create" rule in the Memory Management Policy that the internal docs link to would seem to suggest that.

    #[cfg(feature = "private")]
    pub unsafe fn vendor(&self) -> &str {
        let name = msg_send![self, vendorName];
        crate::nsstring_as_str(name)
    }

Is there a safety contract for these private functions? It doesn't seem like there is actually any unsafe being used. If so, I will happily add the docs for them.

waywardmonkeys commented 3 months ago

@tim-harding My changes landed so this now has the promised conflicts!

tim-harding commented 3 months ago

I don't think I'm likely to work on this further for the time being. Thank you for your time.