Use OsString for process names
#1196
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
As previously mentioned in the other PR, the ergonomics of |
|
Just so you know: didn't forget, just very busy. I'll try to come back to your PR in the next weeks or so. |
GuillaumeGomez
approved these changes
Mar 18, 2024
|
Sorry for the delay, looks all good to me! Can you fix the merge conflicts please? Then I'll merge. If I remember correctly you wanted to make similar changes in other parts of the API? If so after this PR is merged, please send what's next. :) |
CryZe
force-pushed
the
process-name-os-string
branch
from
8e23905
to
836897d
Compare
Previously the stat file on Linux was parsed under the assumption that it contains valid UTF-8. This is not necessarily the case for two reasons: 1. Process names don't need to be valid UTF-8 at all on Linux. They can contain invalid bytes. 2. Even if the process name was valid UTF-8, the assumption breaks because the process name is limited to 15 characters, which can cut a valid code point in half. This causes the stat file to not be parseable, causing the entire process not to show up in the list. So even if you aren't using the name of the process at all, you can't find it as part of the list. One solution is to lossily convert the process name to a string. However this means that the [Unicode replacement character `�`](https://www.fileformat.info/info/unicode/char/fffd/index.htm) is now part of those process names. The character when encoded as UTF-8 is 3 bytes. This means that process names can now be longer than 15 bytes, or in other words, doing a name based comparison on the first 15 bytes, such as suggested in the documentation of the crate, is no longer easily possible. The solution is to just provide the name as an `OsString` instead, keeping the bytes around as is.
CryZe
force-pushed
the
process-name-os-string
branch
from
836897d
to
9c160ff
Compare
|
Alright, rebased and CI is green. |
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously the stat file on Linux was parsed under the assumption that it contains valid UTF-8. This is not necessarily the case for two reasons:
This causes the stat file to not be parseable, causing the entire process not to show up in the list. So even if you aren't using the name of the process at all, you can't find it as part of the list.
One solution is to lossily convert the process name to a string. However this means that the Unicode replacement character
�is now part of those process names. The character when encoded as UTF-8 is 3 bytes. This means that process names can now be longer than 15 bytes, or in other words, doing a name based comparison on the first 15 bytes, such as suggested in the documentation of the crate, is no longer easily possible.The solution is to just provide the name as an
OsStringinstead, keeping the bytes around as is.Resolves #1190