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

Update sysinfo version #921

Merged
merged 3 commits into from
Jan 25, 2022
Merged

Conversation

GuillaumeGomez
Copy link
Contributor

In the new sysinfo version, I turned Pid into a newtype.

@dandavison
Copy link
Owner

Hi @GuillaumeGomez, thanks very much for doing this. It looks like there are some relevant test failures, but let us know if you'd like us to finalise the PR now.

@GuillaumeGomez
Copy link
Contributor Author

Ah indeed! I'll fix the PR as soon as possible then.

@GuillaumeGomez GuillaumeGomez force-pushed the update-sysinfo branch 3 times, most recently from 4691062 to 9f21643 Compare January 20, 2022 10:17
@GuillaumeGomez
Copy link
Contributor Author

The clippy lints seem buggy so just in case I put them into their own commit.

@GuillaumeGomez
Copy link
Contributor Author

CI is happy! \o/

@dandavison
Copy link
Owner

Thanks @GuillaumeGomez. Would it also be an option for delta to work with native integer types, and only convert to the sysinfo::Pid type at the sites where delta is calling the library?

Perhaps another way of asking my question is -- I'm curious, can you explain why the sysinfo::Pid type needs to be used by clients of the sysinfo library, rather than being private to the library?

@GuillaumeGomez
Copy link
Contributor Author

Thanks @GuillaumeGomez. Would it also be an option for delta to work with native integer types, and only convert to the sysinfo::Pid type at the sites where delta is calling the library?

Pid implements From where the conversion is from the native integer.

Perhaps another way of asking my question is -- I'm curious, can you explain why the sysinfo::Pid type needs to be used by clients of the sysinfo library, rather than being private to the library?

The problem is that this native type isn't the same on all platforms, making it complicated to write one code to handle all platforms at once. With this wrapping, it's now possible. I added a common fallback with from_u32 and as_u32, like that if you really need to work on the wrapped type, you can.

Comment on lines 894 to 896
(Pid::from_u32(2), 100, "-shell", None),
(
Pid::from_u32(3),
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about moving all the Pid::from_u32(..) calls into MockProcInfo::with(..)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think @dandavison ?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I'd also prefer to continue to use native integers where possible, and only wrap as sysinfo::Pid where necessary -- i.e. close to the library call boundaries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine by me! Will sent an update as soon as possible.

@GuillaumeGomez
Copy link
Contributor Author

I removed the sysinfo::Pid usage as much as possible, making the changes much smaller.

@dandavison dandavison merged commit 9436b05 into dandavison:master Jan 25, 2022
@dandavison
Copy link
Owner

Thanks a lot @GuillaumeGomez!

@GuillaumeGomez GuillaumeGomez deleted the update-sysinfo branch January 25, 2022 15:10
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.

3 participants