Times built-in #351
Times built-in #351
Conversation
WalkthroughThe Yash shell has been enhanced with a new Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (7)
- yash-builtin/src/lib.rs (2 hunks)
- yash-builtin/src/times.rs (1 hunks)
- yash-builtin/src/times/format.rs (1 hunks)
- yash-builtin/src/times/syntax.rs (1 hunks)
- yash-env/src/system.rs (3 hunks)
- yash-env/src/system/real.rs (3 hunks)
- yash-env/src/system/virtual.rs (4 hunks)
Additional comments: 12
yash-builtin/src/times/syntax.rs (3)
- 27-37: The
Errorenum design for handling parsing errors in thetimesbuilt-in command is well-structured and follows Rust's standard practices for error handling.- 39-55: The implementation of
MessageBaseforErroris correctly done, providing clear and concise error messages and annotations for different types of parsing errors.- 57-67: The
parsefunction is well-implemented, correctly handling the parsing of command line arguments for thetimesbuilt-in command, including the validation of no operands and options.yash-builtin/src/times.rs (1)
- 82-95: The
mainfunction for thetimesbuilt-in command is correctly implemented, effectively parsing command line arguments, retrieving system times, formatting the output, and handling errors.yash-builtin/src/times/format.rs (1)
- 55-108: The tests for the formatting functions in the
timesmodule are comprehensive and well-structured, covering a wide range of scenarios to ensure correctness.yash-builtin/src/lib.rs (2)
- 78-79: The addition of the
timesmodule to the list of built-in command modules is correctly done.- 269-275: The integration of the
timesbuilt-in command into theBUILTINSarray is correctly done, maintaining the alphabetical order and specifying the appropriate execute function.yash-env/src/system/real.rs (1)
- 265-286: The implementation of the
timesmethod in theRealSystemis correctly done, using safe practices for uninitialized memory, error handling, and type conversions to retrieve and return system times.yash-env/src/system.rs (1)
- 403-425: The definition of the
Timesstruct is well-structured and aligns with the PR objectives by representing consumed CPU time values. The inclusion of fields for both the current process and its children, as well as theticks_per_secondfield for converting clock ticks to seconds, is thoughtful and comprehensive. This struct will serve as the return type for thetimesmethod, facilitating detailed time tracking and analysis.One minor suggestion for improvement is to include documentation examples demonstrating how to use the
Timesstruct, especially the conversion of clock ticks to seconds. This would enhance the usability and understandability of the new feature for developers and users alike.Consider adding documentation examples that demonstrate how to use the
Timesstruct, particularly focusing on converting clock ticks to seconds. This will improve the usability and clarity of the new time tracking functionality.yash-env/src/system/virtual.rs (3)
- 68-68: The import of the
Timesstruct is correctly added to support the new time tracking functionality. This ensures that theTimesstruct is accessible within this module for implementing the associated logic.- 1055-1055: The addition of the
timesfield in theSystemStatestruct is crucial for storing consumed CPU time. This aligns with the PR objectives to enhance time tracking capabilities within the Yash shell. It's important to ensure that this field is properly initialized and managed throughout the system's lifecycle to accurately reflect CPU time consumption.- 619-622: The implementation of the
timesmethod in theVirtualSystemstruct is a key component of the new time tracking functionality. This method provides a way to retrieve the consumed CPU time, encapsulated within theTimesstruct, from theSystemState. It's essential that this method correctly handles any potential errors and returns theTimesstruct as expected. The current implementation directly returns thetimesfield fromSystemState, assuming successful retrieval. Given the context of a virtual system primarily used for testing, this straightforward approach seems appropriate. However, in a more complex or real-world scenario, additional error handling or validation might be necessary.
| /// Returns consumed CPU times. | ||
| fn times(&self) -> nix::Result<Times>; |
There was a problem hiding this comment.
The addition of the times method to the System trait is a crucial enhancement for supporting the new time tracking functionality. This method is expected to return consumed CPU times, which aligns with the PR objectives. However, it's important to ensure that all implementors of the System trait provide a meaningful implementation of this method to avoid runtime errors or unimplemented method panics.
Ensure that all concrete implementations of the System trait (e.g., RealSystem, VirtualSystem) provide an implementation for the times method. This is crucial for maintaining the integrity of the system and avoiding runtime issues.
yash-builtin/src/times/format.rs
Outdated
| /// Formats a single time. | ||
| /// | ||
| /// This function panics if the `ticks_per_second` is zero. | ||
| fn format_one_time<W>(ticks: u64, ticks_per_second: u64, result: &mut W) -> std::fmt::Result | ||
| where | ||
| W: std::fmt::Write, | ||
| { | ||
| let seconds = ticks / ticks_per_second; | ||
| let minutes = seconds / 60; | ||
| let sub_minute_ticks = ticks - minutes * 60 * ticks_per_second; | ||
| let seconds = sub_minute_ticks as f64 / ticks_per_second as f64; | ||
| write!(result, "{minutes}m{seconds:.6}s") | ||
| } |
There was a problem hiding this comment.
The format_one_time function correctly formats time values but panics if ticks_per_second is zero. Consider handling this case more gracefully, perhaps by returning an error, to improve robustness.
This change makes the `Times` struct contain seconds in f64 instead of clock ticks in u64. The `tms` struct returned by the underlying `times` function contains values in `clock_t`. The previous implementation assumed that it is an integral type, but POSIX allows it to be a floating-point type. This change makes the `Times` struct use f64 for all time values, which can be cast from any types `clock_t` may be.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- yash-builtin/src/times/format.rs (1 hunks)
- yash-env/src/system.rs (3 hunks)
- yash-env/src/system/real.rs (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- yash-builtin/src/times/format.rs
- yash-env/src/system.rs
Additional comments: 3
yash-env/src/system/real.rs (3)
- 36-36: The import of the
Timesstruct indicates its use within this file, likely for representing time values related to the newtimesmethod. Ensure that theTimesstruct is well-documented and its fields are appropriately named to reflect their purpose clearly.- 58-58: The use of
MaybeUninitis appropriate for handling uninitialized data structures in Rust, especially when dealing with FFI. This usage is a good practice for safely managing memory that will be initialized by external library calls.- 262-288: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-284]
Overall, the file follows Rust best practices for FFI and system interactions. The new
timesmethod is correctly implemented with appropriate safety checks. Ensure that all new code is covered by unit tests, especially to verify the correct behavior of thetimesmethod under various system conditions.
Summary by CodeRabbit
timesbuilt-in command to the Yash shell for tracking and displaying accumulated user and system times.timescommand output, ensuring times are displayed in a human-readable format.Timesstruct for representing CPU time values and methods for retrieving these values from the system.