gz / rust-cpuid

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

update `bitflags` dependency to 2.0 #152

Closed semiviral closed 1 year ago

semiviral commented 1 year ago

As title says, simply updates the bitflags dependency to 2.0, and fixes up the code for any of the API changes from 1.2 -> 2.0.

Namely, bitflags structs no longer implicitly define the following attributes:

#[repr(transparent)]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]

These are re-implemented manually to reflect the proper derivations for the various bitflags types in the crate.

Additionally, many of the direct bitflags type constructors were broken:

let example = ExampleBitflags { bits: 0xdeadbeef };
// ... has been updated to ...
let example = ExampleBitflags::from_bits_truncate(0xdeadbeef);

While this is an effective behaviour change, the old behaviour can be retained via ::from_bits_retain; however, this would only need to be used in cases where we fail to implement all of the bits of a specific CPUID bitflag (so, shouldn't be a concern with proper code coverage).

codecov[bot] commented 1 year ago

Codecov Report

Merging #152 (c1b1273) into master (a837dba) will decrease coverage by 4.60%. The diff coverage is 16.12%.

@@            Coverage Diff             @@
##           master     #152      +/-   ##
==========================================
- Coverage   39.34%   34.75%   -4.60%     
==========================================
  Files           4        4              
  Lines        4072     4610     +538     
==========================================
  Hits         1602     1602              
- Misses       2470     3008     +538     
Impacted Files Coverage Δ
src/extended.rs 69.81% <0.00%> (-13.55%) :arrow_down:
src/lib.rs 42.55% <21.73%> (-8.71%) :arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

semiviral commented 1 year ago

Interestingly, updating to the usage of ::from_bits_truncate over the old direct constructor has revealed a lack of coverage! Some of the requisite bits in the XCR0 & IA32_XSS MSR bitflags were missing (and, interestingly again, incorrect on Wikipedia).

The requisite bits have been put in their respective places, and all tests now pass.

gz commented 1 year ago

Hi @semiviral Thanks a lot for this PR. This looks great.

While this is an effective behaviour change, the old behaviour can be retained via ::from_bits_retain; however, this would only need to be used in cases where we fail to implement all of the bits of a specific CPUID bitflag (so, shouldn't be a concern with proper code coverage).

My thoughts on this is that from_bits_retain may be better for use-cases where one has a newer CPU (with bits that are set but not yet supported in the library). This can happen quite often, in this case the user can still get the value manually (when we expose it) and it doesn't get discarded. Any thoughts on this? Am I missing something?

semiviral commented 1 year ago

Going with from_bits_retain by default would mean that we likely won't notice when new bits are added, or simply when our coverage isn't complete with the existing bits.

I suppose it's a trade-off? Either we truncate and lose the surety that our bit patterns are correct, or we panic when we've missed something (and hopefully implement it). Whichever you think is better, I'll run with.

gz commented 1 year ago

I checked the code and I think from_bits_truncate is fine because we never expose the members anyways to the user so they wouldn't be able to query the bits that got truncated in the first place. Will merge this thanks a lot!

gz commented 1 year ago

I rebased this to latest master and merge it, thanks!