Add some unsafe magic for minimal optimizations, add .idea to .gitignore and fix some typos #71
Conversation
| } | ||
|
|
||
| // Safety: Prior to transmuting we checked if value is bigger than 2 |
There was a problem hiding this comment.
Can you add the mapping (number to enum) as a comment to here? Let's not also remove the knowledge we have since it might not be obvious at the first glance.
| } | ||
|
|
||
| // Safety: Prior to transmuting we checked if value is bigger than 6 | ||
| // and returned the default in that case. |
There was a problem hiding this comment.
Can you add the mapping as a comment here as well?
|
Also can you fix clippy errors? |
src/lib.rs
Outdated
|
|
||
| // Safety: Prior to transmuting we checked if value is bigger than 2 | ||
| // and returned the default in that case. | ||
| unsafe { std::mem::transmute(value) } |
There was a problem hiding this comment.
It appears that it is extremely unsafe. Ref: https://x.com/tenellous/status/1819506534675894296
Is there any benchmark result you could share with us, that makes this change/risk worthy to land?
|
I added tests and comments that illustrate which number maps to which enum variant. Regarding the clippy errors, I don't get any. Now let's talk about performance: If you still think its risky, just deny the commit, it's alright for me. |
| // Safety: Prior to transmuting we checked if value is bigger than 2 | ||
| // and returned the default in that case. | ||
| // | ||
| // Mapping: | ||
| // - 0 => Domain | ||
| // - 1 => IPV4 | ||
| // - 2 => IPV6 | ||
| // - any other number maps to NotSpecial | ||
| unsafe { std::mem::transmute(value as u32) } |
There was a problem hiding this comment.
| // Safety: Prior to transmuting we checked if value is bigger than 2 | |
| // and returned the default in that case. | |
| // | |
| // Mapping: | |
| // - 0 => Domain | |
| // - 1 => IPV4 | |
| // - 2 => IPV6 | |
| // - any other number maps to NotSpecial | |
| unsafe { std::mem::transmute(value as u32) } | |
| match value { | |
| 0 => Self::Domain, | |
| 1 => Self::IPV4, | |
| 2 => Self::IPV6, | |
| _ => Self::Domain, | |
| } |
If you replace the transmute with the same match block as before, the generated code (and therefore the performance) will be identical: https://x.com/tenellous/status/1819528511201464821
I'm all for performance improvement, but here there's only unsafe risk without any benefit.
There was a problem hiding this comment.
Rejecting due to https://x.com/tenellous/status/1819528511201464821?s=46
|
Oh ok I should have done the assembly check myself. Thanks for pointing out that it produces the same exact instructions. |
Thanks for the amazing contribution to open source url parsing!
I am using this lib right now and thought that I should look into it a bit,
I ended up doing minor changes.
I understand if the unsafe code gets rejected, but I even left some safety comments.