-
Notifications
You must be signed in to change notification settings - Fork 40
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
More granular display features #138
Conversation
Codecov Report
@@ Coverage Diff @@
## master #138 +/- ##
==========================================
- Coverage 39.34% 38.66% -0.68%
==========================================
Files 4 4
Lines 4072 4143 +71
==========================================
Hits 1602 1602
- Misses 2470 2541 +71
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
hey, sounds good. Looks like I will need to do a closer look into this; AFAICT if the underlying issue of the serialization stuff calling cpuid is gone, we wouldn't need 3 features |
Agreed! To give a little more context, there were two different limitations that led to this approach:
|
Thanks for the detailed write up. I feel like the serialize use-case would best be fixed by:
Once the serialize/deserialize is gone from all data-structures that might help with aarch64 as we no longer need Default for CpuIdReader. We could probably just make Maybe I need to make Let me know if you think this is sensible. |
I think making the /// Implements function to read/write cpuid.
/// This allows to conveniently swap out the underlying cpuid implementation
/// with one that returns data that is deterministic (for unit-testing).
pub trait CpuIdReader: Copy {
fn cpuid1(&self, eax: u32) -> CpuIdResult {
self.cpuid2(eax, 0)
}
fn cpuid2(&self, eax: u32, ecx: u32) -> CpuIdResult;
}
impl<F> CpuIdReader for F
where
F: Fn(u32, u32) -> CpuIdResult + Copy,
{
fn cpuid2(&self, eax: u32, ecx: u32) -> CpuIdResult {
self(eax, ecx)
}
} This would also allow an "offline" reader that pulls from a Serialization is a bit trickier – One option would be to declare the canonical "ship it over the network" form to be (If we did this, I'm not sure if we'd also want to remove I've implemented these ideas in a new branch, which looks pretty clean. |
This looks great, I like it. Also seems like this change should basically require no changes from the users which is great -- though it might still need a major release. I'm not sure about it need to check the rules again. Anyways, this crate is already at v10 so I guess it's not a big deal ;) I added this commit a9af43c which makes sure tests run again and
I think I added it mostly for testing so I wouldn't forget to make new things serializable. I'm happy to get rid of it completely. I think having Serialize/Deserialize on anything that has a reader in the struct is a bad idea from the start. Hence my suggestion is to only Serialize/Deserialize the |
Okay, that all makes sense to me! I'm going to close this PR, since you're already staging new work on top of my branch. Feel free to tag me for a final review once you're ready to merge, and I can check against Humility and on AArch64. Thanks for the help on this! |
Pushed some changes to the branch that we discussed (remove serialization and json), please have a look this might now be in a usable state for you. One remaining thing I noticed, the library code now contains the |
It looks like we can remove the One remaining issue with your branch: /// The main type used to query information about the CPU we're running on.
///
/// Other structs can be accessed by going through this type.
#[derive(Clone, Copy)]
pub struct CpuId<R: CpuIdReader = CpuIdReaderNative> { Specifying this default means that it doesn't build on AArch64, which lacks It's not pretty, but it turns out we could conditionally add this default parameter: pub struct CpuId<
#[cfg(any(
all(target_arch = "x86", not(target_env = "sgx"), target_feature = "sse"),
all(target_arch = "x86_64", not(target_env = "sgx"))
))]
R: CpuIdReader=CpuIdReaderNative,
#[cfg(not(any(
all(target_arch = "x86", not(target_env = "sgx"), target_feature = "sse"),
all(target_arch = "x86_64", not(target_env = "sgx"))
)))]
R: CpuIdReader
> {
// ... I've done this in commit 65a1144 Finally, I realized that All of these commits are in the In case you're curious, here's the Humility cleanup that this unlocks. In particular, the added flexibility of the |
Ah good catch with the CpuIdReaderNative. Now I'm actually not sure anymore if it is a good idea to have this default even for x86. I fear it might make downstream code harder to cross-compile as it means anyone that uses CpuId as an argument somewhere now has to plaster the same I might drop the commit which means potentially slightly more work for users migrating to v11. |
Thanks for merging in #137!
While trying to integrate it, I realized that it wasn't quite fine-grained enough:
display
feature includesfn raw(...)
andfn json(...)
, which both require an x86 CPU on the machine building the crate (the former because it callscpuid!()
directly; the latter because of how serialization is implemented)This PR splits
display
into three features:display-raw
,display-json
anddisplay-markdown
, whose behavior is exactly what you'd expect 😄This allows me to build with just
display-markdown
, which compiles fine on an AArch64 machine.Codewise, there are no logical changes, but I ended up moving the markdown helper functions into
fn markdown
itself, to avoid having a ton of conditional compilation directives.