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

fix: Address most Clippy warnings #311

Closed
wants to merge 1 commit into from

Conversation

tim-harding
Copy link

These categories of warnings were addressed:

  • Automatic fixes from Clippy (cargo clippy --fix), such as removing redundant references
  • Replace transmutes with safe pointer casts
  • Remove lets that just get returned
  • Add Default impl for types with new()
  • Use iterators instead of indices
  • Add Send and Sync bounds to dyn traits in Arc

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.

These categories of warnings were addressed:

- Automatic fixes from Clippy (`cargo clippy --fix`), such as removing
  redundant references
- Replace transmutes with safe pointer casts
- Remove lets that just get returned
- Add `Default` impl for types with `new()`
- Use iterators instead of indices
- Add `Send` and `Sync` bounds to `dyn` traits in `Arc`
@MarijnS95
Copy link

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

@waywardmonkeys
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Contributor

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

@tim-harding
Copy link
Author

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

@tim-harding tim-harding closed this Jun 3, 2024
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

4 participants