Allow SharedSystem to be used behind a non-mutable reference
#378
Conversation
To keep the system module focused on the System trait, this commit extracts the SelectSystem struct and related items to a new file.
WalkthroughThe changes enhance asynchronous operations in the Changes
Poem
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 (
|
This commit updates the `SharedSystem` methods `set_nonblocking`, `reset_nonblocking`, `read_async`, `write_all`, and `print_error` to take immutable references (`&self`) instead of mutable references (`&mut self`). This change is made to reflect the fact that these methods do not mutate the `SharedSystem` instance itself, but rather the `SelectSystem` instance that it contains. Previously, when you have a non-mutable reference to a `SharedSystem`, you had to clone it to call these methods. Now, you can call them directly.
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- yash-env/CHANGELOG.md (1 hunks)
- yash-env/src/system.rs (7 hunks)
- yash-env/src/system/select.rs (1 hunks)
- yash-env/src/system/shared.rs (1 hunks)
Additional context used
Learnings (1)
yash-env/CHANGELOG.md (2)
Learnt from: magicant PR: magicant/yash-rs#367 File: yash-env/src/system.rs:57-57 Timestamp: 2024-06-08T02:51:51.839Z Learning: The `yash-env/src/subshell.rs` module depends on `SigSet`, which requires `nix::sys::signal::Signal`. Plans are in place to remove `SigSet` in a future PR, and some existing methods like `tcsetpgrp_with_block` still depend on `nix::sys::signal::Signal`.Learnt from: magicant PR: magicant/yash-rs#367 File: yash-env/src/subshell.rs:40-40 Timestamp: 2024-06-08T02:49:23.406Z Learning: The `yash-env/src/subshell.rs` module depends on `SigSet`, which requires `nix::sys::signal::Signal`. Plans are in place to remove `SigSet` in a future PR.
Markdownlint
yash-env/CHANGELOG.md
23-23: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
28-28: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
30-30: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
32-32: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
33-33: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
34-34: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
Additional comments not posted (15)
yash-env/src/system/select.rs (6)
1-16: File header looks good.The licensing information is correctly formatted.
17-63: Struct definition looks good.The
SelectSystemstruct is well-defined and the fields are appropriately documented.
65-76: Trait implementations look good.The
DerefandDerefMuttrait implementations forSelectSystemare correct.
78-221: Method implementations look good.The methods for
SelectSystemare well-documented and follow standard practices. The logic appears correct and efficient.
224-463: Struct and method definitions look good.The
AsyncIo,FdAwaiter,AsyncTime,Timeout, andAsyncSignalstructs, along with their methods, are well-defined and follow standard practices. The logic appears correct and efficient.
466-600: Test cases look good.The test cases for the new components are comprehensive and cover various scenarios. They follow standard testing practices.
yash-env/src/system/shared.rs (4)
1-16: File header looks good.The licensing information is correctly formatted.
17-121: Struct definition looks good.The
SharedSystemstruct is well-defined and the fields are appropriately documented.
123-307: Method implementations look good.The methods for
SharedSystemare well-documented and follow standard practices. The logic appears correct and efficient.
310-679: Trait implementations look good.The
SystemandSignalSystemtrait implementations forSharedSystemare correct and follow standard practices.yash-env/src/system.rs (5)
Line range hint
1-708:
Overall consistency of the file.The file is consistent with the changes including the addition of new modules and usages, and the removal of
SharedSystem.
37-38: Usages ofSelectSystemandSignalStatusadded.The addition of the usages of
SelectSystemandSignalStatuslooks good.Verify the usages of
SelectSystemandSignalStatusto ensure they are correctly implemented.Verification successful
Usages of
SelectSystemandSignalStatusare correctly implemented.The usages of
SelectSystemandSignalStatushave been verified across multiple files and appear to be correctly implemented.
SelectSystemis defined and used insystem.rs,select.rs,shared.rs,virtual.rs, andprocess.rs.SignalStatusis defined and used insystem.rs,select.rs, andshared.rs.All usages align with their respective imports and definitions.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usages of `SelectSystem` and `SignalStatus`. # Test: List the usages of `SelectSystem` and `SignalStatus`. Expect: Correct implementation of the usages. rg --type rust 'SelectSystem' rg --type rust 'SignalStatus'Length of output: 4794
39-39: Removal ofSharedSystem.The removal of
SharedSystemlooks good.Verify the impact of the removal of
SharedSystemto ensure it does not affect the functionality.
707-708: Tests updated to useSharedSystem.The updates to the tests to use
SharedSystemlook good.Verify the correctness of the updated tests to ensure they correctly use
SharedSystem.Verification successful
Verified the usage of
PollandDuration.The imports
std::task::Pollandstd::time::Durationare correctly used in the codebase, particularly inyash-env/src/system.rsand related files. Their usage in various assertions and logic confirms their necessity.
Pollis used in multiple assertions and logic checks.Durationis used in time calculations and comparisons.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the updated tests. # Test: Run the updated tests. Expect: All tests pass. cargo testLength of output: 85
Script:
#!/bin/bash # Searching for the usage of `Poll` in the codebase rg 'Poll' # Searching for the usage of `Duration` in the codebase rg 'Duration'Length of output: 6076
23-24: Modulesselectandsharedadded.The addition of the
selectandsharedmodules looks good.Verify the contents of the new modules to ensure they are correctly implemented.
Verification successful
Modules
selectandsharedare correctly implemented.The contents of the
select.rsandshared.rsmodules have been verified and are correctly implemented with detailed structures and functions.
select.rsimplementsSelectSystemwith extensive functionalities for handling asynchronous I/O, signals, and timers.shared.rsimplementsSharedSystemwith detailed methods for asynchronous interactions and system operations.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the contents of the new modules `select` and `shared`. # Test: List the contents of the new modules. Expect: Correct implementation of the modules. fd select.rs fd shared.rsLength of output: 89
Script:
#!/bin/bash # Description: Verify the contents of the new modules `select` and `shared`. # Test: Display the contents of the new modules to ensure correct implementation. cat yash-env/src/system/select.rs cat yash-env/src/system/shared.rsLength of output: 44430
| - `impl System for &SharedSystem` and `impl trap::SignalSystem for &SharedSystem` | ||
| - This allows `SharedSystem` to be used as a system behind a non-mutable reference. |
There was a problem hiding this comment.
Fix unordered list indentation.
The indentation for the unordered list items is incorrect.
- - This allows `SharedSystem` to be used as a system behind a non-mutable reference.
+ - This allows `SharedSystem` to be used as a system behind a non-mutable reference.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `impl System for &SharedSystem` and `impl trap::SignalSystem for &SharedSystem` | |
| - This allows `SharedSystem` to be used as a system behind a non-mutable reference. | |
| - `impl System for &SharedSystem` and `impl trap::SignalSystem for &SharedSystem` | |
| - This allows `SharedSystem` to be used as a system behind a non-mutable reference. |
Tools
Markdownlint
23-23: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
| - All inherent methods of `SharedSystem` now take `&self` instead of `&mut self`: | ||
| - `SharedSystem::read_async` | ||
| - `SharedSystem::write_all` | ||
| - `SharedSystem::print_error` |
There was a problem hiding this comment.
Fix unordered list indentation.
The indentation for the unordered list items is incorrect.
- - `SharedSystem::read_async`
- - `SharedSystem::write_all`
- - `SharedSystem::print_error`
+ - `SharedSystem::read_async`
+ - `SharedSystem::write_all`
+ - `SharedSystem::print_error`Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - All inherent methods of `SharedSystem` now take `&self` instead of `&mut self`: | |
| - `SharedSystem::read_async` | |
| - `SharedSystem::write_all` | |
| - `SharedSystem::print_error` | |
| - All inherent methods of `SharedSystem` now take `&self` instead of `&mut self`: | |
| - `SharedSystem::read_async` | |
| - `SharedSystem::write_all` | |
| - `SharedSystem::print_error` |
Tools
Markdownlint
32-32: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
33-33: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
34-34: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
Summary by CodeRabbit
New Features
SelectSystemfor asynchronous I/O, signal handling, and timers coordination.AsyncIo,AsyncTime, andAsyncSignal.Bug Fixes
Refactor
SharedSystemmethods intoSelectSystem.Tests