Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update bitflags dependency to 2.0 #152

Closed
wants to merge 4 commits into from
Closed

Conversation

semiviral
Copy link
Contributor

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
Copy link

codecov bot commented Apr 2, 2023

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%) ⬇️
src/lib.rs 42.55% <21.73%> (-8.71%) ⬇️

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

@semiviral
Copy link
Contributor Author

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
Copy link
Owner

gz commented Apr 7, 2023

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
Copy link
Contributor Author

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
Copy link
Owner

gz commented Apr 17, 2023

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
Copy link
Owner

gz commented Apr 17, 2023

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

@gz gz closed this Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants