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

Implement some standard traits #254

Merged
merged 1 commit into from
May 13, 2023
Merged

Implement some standard traits #254

merged 1 commit into from
May 13, 2023

Conversation

tatref
Copy link
Contributor

@tatref tatref commented Feb 19, 2023

I implemented a few standard traits, but there's probably others.

For Shm, some field like rss or atime are not really part of the indentity of Shm, so I left them out

@tatref tatref marked this pull request as draft February 25, 2023 23:18
@tatref
Copy link
Contributor Author

tatref commented Feb 25, 2023

Converted to draft, as the Hash for Shm is probably plain wrong if some fields are not part of the hash

@eminence
Copy link
Owner

The derived traits seem OK to me, but I agree that the custom impl for Shm seems a little suspect. For me, the primary purpose to have a type impl Hash is to be able to use it has a key in a HashMap. I guess it's not inconceivable that you'd want to do this. Did you have a specific usecase for implementing Hash on Shm ?

@tatref
Copy link
Contributor Author

tatref commented Feb 26, 2023

I'm working on a tool for memory diagnostics.
I want to store a list of physical pages used by each shared memory segment, so HashMap<Shm, Vec<Pfn>> makes a lot of sense.

I'll rework the PR to remove the custom impl stuff.

@eminence
Copy link
Owner

I'm not saying that procfs can't implement these, but could you put Shm in your own local wrapper type, and impl Hash and PartialEq according to the rules that make sense for your tool?

@tatref
Copy link
Contributor Author

tatref commented Feb 26, 2023

After some thinking, I believe that the custom Hash and PartialEq that I initially implemented is plain wrong.
I switched to using derive

Tell my if it's good for you

@tatref tatref marked this pull request as ready for review February 28, 2023 07:56
@tatref
Copy link
Contributor Author

tatref commented May 10, 2023

I rebased from master

@eminence eminence merged commit e54f13e into eminence:master May 13, 2023
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.

2 participants