Skip to content

Misc cleanup: pub(crate) mock fields, log_println allow, current_processor_number, stdin guard #755

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.

Sign up for GitHub

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

Open
wdcui wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Misc cleanup: pub(crate) mock fields, log_println allow, current_processor_number, stdin guard #755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions litebox/src/platform/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ impl IPInterfaceProvider for MockPlatform {

#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
pub(crate) struct MockInstant {
time: u64,
pub(crate) time: u64,
}

impl Instant for MockInstant {
Expand All @@ -230,7 +230,7 @@ impl Instant for MockInstant {
}

pub(crate) struct MockSystemTime {
time: u64,
pub(crate) time: u64,
Comment on lines 213 to +233
Copy link
Copy Markdown
Member

@jaybosamiya-ms jaybosamiya-ms Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

impl SystemTime for MockSystemTime {
Expand Down Expand Up @@ -290,6 +290,9 @@ impl RawPointerProvider for MockPlatform {

impl StdioProvider for MockPlatform {
fn read_from_stdin(&self, buf: &mut [u8]) -> Result<usize, StdioReadError> {
if buf.is_empty() {
return Ok(0);
}
let Some(front) = self.stdin_queue.write().unwrap().pop_front() else {
return Err(StdioReadError::Closed);
};
Expand Down
12 changes: 12 additions & 0 deletions litebox/src/platform/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,13 @@ pub use page_mgmt::PageManagementProvider;
#[macro_export]
macro_rules! log_println {
($platform:expr, $s:expr) => {{
#[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)]
Comment on lines +26 to +32
Copy link
Copy Markdown
Member

@jaybosamiya-ms jaybosamiya-ms Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

use $crate::platform::DebugLogProvider as _;
let mut t: arrayvec::ArrayString<8192> = arrayvec::ArrayString::new();
writeln!(t, $($tt)*).unwrap();
Expand Down Expand Up @@ -666,6 +668,16 @@ pub trait SystemInfoProvider {
/// Return `Some(address)` if the VDSO is available on the platform, or `None`
/// if the platform does not support or provide a VDSO.
fn get_vdso_address(&self) -> Option<usize>;

/// 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
}
Comment on lines +671 to +680
Copy link
Copy Markdown
Member

@jaybosamiya-ms jaybosamiya-ms Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/// A provider for thread-local storage.
Expand Down
Loading