add From<&str> For InfoKind#262
Conversation
Signed-off-by: Gris Ge <cnfourt@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #262 +/- ##
==========================================
- Coverage 68.10% 67.90% -0.21%
==========================================
Files 144 146 +2
Lines 10103 10284 +181
==========================================
+ Hits 6881 6983 +102
- Misses 3222 3301 +79 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the InfoKind parsing logic by implementing From<&str> and updating the Parseable trait implementation to utilize it. A review comment identifies a performance regression where unknown link types now undergo a double allocation (String to &str and back to String). The reviewer suggests implementing From<String> for InfoKind to allow moving the string directly into the Other variant, avoiding the unnecessary clone.
| } | ||
| let s = parse_string(buf.value()) | ||
| .context("invalid IFLA_INFO_KIND value")?; | ||
| Ok(s.as_str().into()) |
There was a problem hiding this comment.
The refactoring to use From<&str> introduces a double allocation when parsing an unknown InfoKind (the Other variant).
parse_string returns a String. Calling s.as_str().into() then invokes InfoKind::from(&str), which calls s.to_owned() to create a second String for the Other variant, after which the original String is dropped.
While this only affects unknown link types, it is a performance regression compared to the previous implementation which moved the String into the Other variant. To resolve this while keeping the code clean, you could implement From<String> for InfoKind (which would move the string in the Other case) and use Ok(s.into()) here.
1 participant
No description provided.