GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — no_std clippy by youknowone · Pull Request #7043 · RustPython/RustPython Conversation
|
📝 WalkthroughWalkthroughThis PR enables additional Clippy lints and systematically replaces many Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin no_std |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
44bf6bf to
da9ad35
Compare
| #[cfg(not(windows))] | ||
| { | ||
| // wchar_t is i32 on some platforms and u32 on others | ||
| #[allow(clippy::useless_conversion)] |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| #[allow(clippy::useless_conversion)] | |
| #[allow(clippy::useless_conversion, reason = "wchar_t is i32 on some platforms and u32 on others")] |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| #[cfg(not(windows))] | ||
| { | ||
| // wchar_t is i32 on some platforms and u32 on others | ||
| #[allow(clippy::useless_conversion)] |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Also here
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| let addr = (ptr_value as isize + cur * wchar_size as isize) as *const libc::wchar_t; | ||
| unsafe { | ||
| // wchar_t is i32 on some platforms and u32 on others | ||
| #[allow(clippy::unnecessary_cast)] |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@youknowone you missed this one:)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I ran a project-wide appliance task. Let's handle this there
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
nvm, that task didn't catch this one 🤦
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| #[cfg(not(windows))] | ||
| { | ||
| // wchar_t is i32 on some platforms and u32 on others | ||
| #[allow(clippy::useless_conversion)] |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| let addr = (ptr_value as isize + cur * wchar_size as isize) as *const libc::wchar_t; | ||
| unsafe { | ||
| // wchar_t is i32 on some platforms and u32 on others | ||
| #[allow(clippy::unnecessary_cast)] |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@youknowone you missed this one:)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
In `@crates/vm/src/stdlib/thread.rs`:
- Around line 20-23: The import is pulling fmt from alloc which triggers the
alloc_instead_of_core lint; change the use to import fmt from core (so types
like fmt::Debug, fmt::Formatter, fmt::Result come from core::fmt) while keeping
sync::{Arc, Weak} from alloc; update the use statement that currently reads use
alloc::{ fmt, sync::{Arc, Weak}, }; to import core::fmt instead and leave Arc
and Weak unchanged, and ensure any references to fmt types (Debug, Formatter,
Result) remain correct.
crates/vm/src/stdlib/signal.rs (1)
31-31: Nit: use the existingatomicalias for brevity.Line 15 already imports
core::sync::atomic::{self, Ordering}, so the fully qualifiedcore::sync::atomic::AtomicBoolcan be simplified.♻️ Suggested simplification
- static WAKEUP_IS_SOCKET: core::sync::atomic::AtomicBool = core::sync::atomic::AtomicBool::new(false); + static WAKEUP_IS_SOCKET: atomic::AtomicBool = atomic::AtomicBool::new(false);
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| use alloc::{ | ||
| fmt, | ||
| sync::{Arc, Weak}, | ||
| }; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🏁 Script executed:
# Check the current imports at lines 20-23 sed -n '15,30p' crates/vm/src/stdlib/thread.rs # Search for Clippy lint allow/deny configuration rg -n 'allow|deny|warn' crates/vm/src/stdlib/thread.rs | head -20 # Check if there's a clippy.toml or Cargo.toml with lint configuration find . -name "clippy.toml" -o -name "Cargo.toml" | grep -E "(^./Cargo.toml|clippy)" | head -5 # Check for Clippy lint configuration in the workspace Cargo.toml rg -A 20 '\[lints\]' crates/vm/Cargo.toml
Repository: RustPython/RustPython
Length of output: 932
🏁 Script executed:
# Check the workspace Cargo.toml for lint configuration rg -A 50 '\[lints\]' Cargo.toml # Check for alloc_instead_of_core and std_instead_of_alloc lint settings rg 'alloc_instead_of_core|std_instead_of_alloc' Cargo.toml # Examine what fmt is actually used for in the file rg -n 'fmt::' crates/vm/src/stdlib/thread.rs | head -20
Repository: RustPython/RustPython
Length of output: 570
🏁 Script executed:
# Double-check the full [lints] section to understand the context sed -n '/\[lints\]/,/^\[/p' Cargo.toml | head -50 # Verify this is the workspace lints head -20 Cargo.toml
Repository: RustPython/RustPython
Length of output: 827
🏁 Script executed:
# Find the workspace Cargo.toml and check the [lints] section find . -name "Cargo.toml" -type f | xargs grep -l "workspace = true" | head -1 # Look for [lints] in the root Cargo.toml rg -B 5 -A 20 'alloc_instead_of_core' . # Also check if alloc::fmt is re-exported from core::fmt python3 <<'EOF' # Verify that alloc::fmt re-exports from core::fmt # This is Rust standard library fact-checking print("In Rust standard library:") print("- alloc::fmt re-exports core::fmt and adds format! macro") print("- core::fmt contains: Debug, Formatter, Result, etc.") print("- alloc_instead_of_core lint warns when using alloc for types from core") print("- std_instead_of_alloc lint warns when using std for types from alloc") print("\nFor fmt::Debug, fmt::Formatter, fmt::Result -> these are core::fmt types") print("Using alloc::fmt for these would trigger alloc_instead_of_core lint") EOF
Repository: RustPython/RustPython
Length of output: 961
Use core::fmt instead of alloc::fmt to comply with the alloc_instead_of_core Clippy lint.
The workspace has alloc_instead_of_core = "warn" enabled. Since fmt::Debug, fmt::Formatter, and fmt::Result are core::fmt types (not alloc::fmt), importing from alloc::fmt triggers this lint. The alloc::sync::{Arc, Weak} import is correct.
-use alloc::{ - fmt, - sync::{Arc, Weak}, -}; +use alloc::sync::{Arc, Weak}; +use core::fmt;
‼️ 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.
| use alloc::{ | |
| fmt, | |
| sync::{Arc, Weak}, | |
| }; | |
| use alloc::sync::{Arc, Weak}; | |
| use core::fmt; |
In `@crates/vm/src/stdlib/thread.rs` around lines 20 - 23, The import is pulling
fmt from alloc which triggers the alloc_instead_of_core lint; change the use to
import fmt from core (so types like fmt::Debug, fmt::Formatter, fmt::Result come
from core::fmt) while keeping sync::{Arc, Weak} from alloc; update the use
statement that currently reads use alloc::{ fmt, sync::{Arc, Weak}, }; to import
core::fmt instead and leave Arc and Weak unchanged, and ensure any references to
fmt types (Debug, Formatter, Result) remain correct.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Great!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.