search

gz / rust-cpuid

cpuid library in rust.
https://docs.rs/raw-cpuid/
MIT License
154 stars 45 forks source link

Improve feature coverage of `ExtendedFeatures` #99

Closed zdivelbiss closed 2 years ago

zdivelbiss commented 2 years ago

Adds support for all the missing bits in ExtendedFeatures.ecx. Additionally, all of the functions within the ExtendedFeatures impl are marked const and prefixed with #[inline] for better codegen at compile-time.

gz commented 2 years ago

Hi thanks, this looks great aside from changing the emojis.

I'm curious about the reason for the #[inline] annotations, did you notice some slowdown in your code or is this based on some recommendation or personal experience? I can imagine there may be cases where it can hurt debugging (e.g., you're checking for some feature and then use it after and due to some bug it still crashes, if the function name appears in the backtrace it will help a developer point to the problem quickly whereas now this hint may be gone (not a huge deal but something to consider maybe)).

gz commented 2 years ago

Also do you mind rebasing the commit on top of current master instead of the merge commit? I'd like to ensure a linear history for the project so currently it doesn't let me merge it because of github settings.

zdivelbiss commented 2 years ago

I'm curious about the reason for the #[inline] annotations, did you notice some slowdown in your code or is this based on some recommendation or personal experience? I can imagine there may be cases where it can hurt debugging (e.g., you're checking for some feature and then use it after and due to some bug it still crashes, if the function name appears in the backtrace it will help a developer point to the problem quickly whereas now this hint may be gone (not a huge deal but something to consider maybe)).

I don't believe there's any way the functions I marked as inlineable can meaningfully fail, so the stack trace should almost never make it to the inside of these functions (if they weren't inlined); so I didn't see any harm. But, they aren't specifically necessary, so if you'd like me to remove the annotations I can.

Also do you mind rebasing the commit on top of current master instead of the merge commit? I'd like to ensure a linear history for the project so currently it doesn't let me merge it because of github settings.

I believe this is done now. Evidently, I was many, many commits behind, haha.

gz commented 2 years ago

I don't believe there's any way the functions I marked as inlineable can meaningfully fail, so the stack trace should almost never make it to the inside of these functions (if they weren't inlined); so I didn't see any harm. But, they aren't specifically necessary, so if you'd like me to remove the annotations I can.

No that seems fine to me we can keep it, was just curious about the change!

I believe this is done now. Evidently, I was many, many commits behind, haha.

Thanks! Unfortunately github UI still tells me "This branch cannot be rebased due to conflicts" did something go wrong with the rebasing? May be easier to just checkout master and cherry pick your two commits on top of it again... e.g., I'd rather like to avoid doing a merge commit here (since your branch seemed quite a bit behind) as it now looks like the change has to touch many commits which it really shouldn't given it's size.

gz commented 2 years ago

I cherry picked your commit on top of the current master and it applied cleanly: https://github.com/gz/rust-cpuid/commit/71af655f557c1f510805cf69ac16a2c32eb48a04

Will close this PR

gz commented 2 years ago

Thanks for your contribution!

gz commented 2 years ago

Changes were just released as v10.6.0