-
Notifications
You must be signed in to change notification settings - Fork 128
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
Clean up enumeration on macOS #218
Conversation
As you provided some oft the last fixes for macOS @huntc, it would be great, if you could have a look at this PR as well. |
I'm sorry for bothering you @jessebraham! I failed to click in the right place when asking @eldruin for a review. |
3bff354
to
725424e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! The new code does look better.
Hi. I’ve not looked into the code in any detail, and have no strong opinion on the changes. What I recommend though is confirmation here that the new code has been profiled for leaks. I spent a lot of effort on profiling previously given the nature of the problems being fixed back then. Thanks. |
This is a valid point. How did your setup for profiling this looks like? You are mentioning using Instuments in #98. Can you give some more details on how you profiled the device enumeration on macOS? I have to admit that I have little experience with XCode and its graphical tool chest. Have you used the CLI tool |
A first shot with
The message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thank you!
Friendly ping regarding #218 (comment) @huntc |
Apologies for not replying. I don't recall the exact steps that I took now. I vaguely recall launching xcode for my project from the command line and then instrumenting it. Sorry for not being able to provide more info. |
The code in question still builds fine for aarch64-apple-ios. So there is no obvious reason for not pulling in more convenience.
This reduces verbosity for this function as well.
This transitive dependency is used by some of our examples. The advisory is informational and not about an actual issue. Our MSRV currently prevents us from upgrading to a newer version of clap. When bumping MSRV, we shall look into newer versions of clap to resolve this issue.
They are used with IOKit and there are no reexports of them from core_foundation or io_kit_sys.
It's the transitive dependency atty again which we are pulling in through clap. Same situation as with RUSTSEC-2024-0370, it's just no maintained as of now.
f03e12d
to
d1e8b0e
Compare
Thanks for your feedback! In this case I will take the test made in #218 (comment) as an indicator that I've not introduced new leaks with this refactoring. |
I spotted missing checks for the returned status for several Core Foundation and IOKit function calls. While looking into this and adding checks, I came across the core-foundation crate which comes in handy for getting less verbose with basic operation with Core Foundation types.
The commit history shows the initial adding of checks and the later migration to core-foundation.