-
Notifications
You must be signed in to change notification settings - Fork 3
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
SelectSystem
struct is well-defined and the fields are appropriately documented.
65-76
: Trait implementations look good.The
Deref
andDerefMut
trait implementations forSelectSystem
are correct.
78-221
: Method implementations look good.The methods for
SelectSystem
are 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
, andAsyncSignal
structs, 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
SharedSystem
struct is well-defined and the fields are appropriately documented.
123-307
: Method implementations look good.The methods for
SharedSystem
are well-documented and follow standard practices. The logic appears correct and efficient.
310-679
: Trait implementations look good.The
System
andSignalSystem
trait implementations forSharedSystem
are 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 ofSelectSystem
andSignalStatus
added.The addition of the usages of
SelectSystem
andSignalStatus
looks good.Verify the usages of
SelectSystem
andSignalStatus
to ensure they are correctly implemented.Verification successful
Usages of
SelectSystem
andSignalStatus
are correctly implemented.The usages of
SelectSystem
andSignalStatus
have been verified across multiple files and appear to be correctly implemented.
SelectSystem
is defined and used insystem.rs
,select.rs
,shared.rs
,virtual.rs
, andprocess.rs
.SignalStatus
is 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
SharedSystem
looks good.Verify the impact of the removal of
SharedSystem
to ensure it does not affect the functionality.
707-708
: Tests updated to useSharedSystem
.The updates to the tests to use
SharedSystem
look good.Verify the correctness of the updated tests to ensure they correctly use
SharedSystem
.Verification successful
Verified the usage of
Poll
andDuration
.The imports
std::task::Poll
andstd::time::Duration
are correctly used in the codebase, particularly inyash-env/src/system.rs
and related files. Their usage in various assertions and logic confirms their necessity.
Poll
is used in multiple assertions and logic checks.Duration
is 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
: Modulesselect
andshared
added.The addition of the
select
andshared
modules looks good.Verify the contents of the new modules to ensure they are correctly implemented.
Verification successful
Modules
select
andshared
are correctly implemented.The contents of the
select.rs
andshared.rs
modules have been verified and are correctly implemented with detailed structures and functions.
select.rs
implementsSelectSystem
with extensive functionalities for handling asynchronous I/O, signals, and timers.shared.rs
implementsSharedSystem
with 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
SelectSystem
for asynchronous I/O, signal handling, and timers coordination.AsyncIo
,AsyncTime
, andAsyncSignal
.Bug Fixes
Refactor
SharedSystem
methods intoSelectSystem
.Tests