Misc cleanup: pub(crate) mock fields, log_println allow, current_processor_number, stdin guard#755
Misc cleanup: pub(crate) mock fields, log_println allow, current_processor_number, stdin guard#755
Conversation
…essor_number, stdin guard Small independent cleanups: - Make MockInstant::time and MockSystemTime::time pub(crate) so tests in other modules can construct instances with specific values. - Add #[allow(unused_imports)] to log_println! macro use-statements to suppress warnings when the macro is invoked in contexts where the import is redundant. - Add SystemInfoProvider::current_processor_number() default method returning 0, for platforms that don't expose processor topology. - Add empty-buffer early return in MockPlatform::read_from_stdin() to avoid unnecessarily consuming queued input on zero-length reads.
|
🤖 SemverChecks 🤖 No breaking API changes detected Note: this does not mean API is unchanged, or even that there are no breaking changes; simply, none of the detections triggered. |
jaybosamiya-ms
added
the
expmt:shadow-kiln
| @@ -230,7 +230,7 @@ impl Instant for MockInstant { | |||
| } | |||
|
|
|||
| pub(crate) struct MockSystemTime { | |||
| time: u64, | |||
| pub(crate) time: u64, | |||
There was a problem hiding this comment.
This is actually making them from private into crate-public, which contradicts the claim in the PR description. I don't understand why this change is done/needed.
| #[allow(unused_imports)] | ||
| use $crate::platform::DebugLogProvider as _; | ||
| $platform.debug_log_print($s); | ||
| }}; | ||
| ($platform:expr, $($tt:tt)*) => {{ | ||
| use core::fmt::Write as _; | ||
| #[allow(unused_imports)] |
There was a problem hiding this comment.
I'm not sure I understand why these unused_imports are being marked as allow here. Is there a use case that actually breaks if you don't put the allow?
This part of the change is fine to keep btw, no issue specifically due to the exact scenario it is in (i.e., a macro in a logging utility), but more broadly, introducing allows is a code smell.
|
|
||
| /// Returns the current processor number, used to emulate `getcpu`-family | ||
| /// syscalls and related VDSO interfaces. | ||
| /// | ||
| /// Platforms that do not expose a stable processor identifier, or that | ||
| /// virtualize CPU topology, may return `0`. Callers arrive in subsequent | ||
| /// stacked PRs. | ||
| fn current_processor_number(&self) -> u32 { | ||
| 0 | ||
| } |
There was a problem hiding this comment.
If we are allowing platforms to just give 0, are there scenarios where it matters that we are accurate? Why can't the shim just assume 0 always? Otherwise, this will lead to making the interface wider unnecessarily.
Also, same comment as in other PRs, doc strings should not mention "subsequent stacked PRs" and such.
2 participants
Summary
MockPlatformfieldspub(crate)instead of fully public, since the mock is only used within the crate's test infrastructure.#[allow(unused_imports)]forlog_println!macro import (used conditionally).current_processor_number()toSystemInfoProviderwith a default returning0, forgetcpu-family syscall emulation.Dropguard toMockPlatform::read_stdin()to clear the stdin buffer on return.Split from #743.