-
Notifications
You must be signed in to change notification settings - Fork 624
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
Making code more Clippy-compliant #2624
Conversation
…re: redundant clone, needless return keyword, using a Vec pointer where a slice would work. This should lead to a (minor) overall performance boost, though many of the offences were in test code, not production.
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.
Reviewed runtime
and core
.
pub unsafe fn syscall3(mut n: usize, a1: usize, a2: usize, a3: usize) -> usize { | ||
asm!("syscall" | ||
llvm_asm!("syscall" |
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.
Not sure why this change is needed. CC @olonho
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.
Guess rust-lang/rfcs#2843 hints that this is just the new syntax.
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.
error: use of deprecated item 'asm': the syntax of asm! will change soon, use llvm_asm! to avoid breakage
--> runtime/runtime-params-estimator/src/testbed_runners.rs:107:5
|
107 | asm!("syscall"
| ^^^ help: replace the use of the deprecated item: `llvm_asm`
|
I don't know anything about this asm
macro, but just applied to suggested change. Though I see it failed to compile with CI, so I'll take a closer look at it.
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.
I bumped the version of rust nightly I am using locally to get around a bug in RLS that is fixed in a later version (rust-lang/vscode-rust#749), and this is why I was seeing this deprecation error. The version of nightly the project is currently on does not yet have llvm_asm
. I'll revert this change for now and we can bring it back in if we bump the rust version for the whole project.
By the way, what are others using for code navigation? I assume I must be the only one using RLS in VSCode if there is an issue with it in the nightly version the project is currently on.
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.
Many people use CLion, we can get you a license.
I think @nearmax was making sure VSCode also works though.
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.
I stopped using VSCode because I was lagging way too much, because I was using it for remote development (running RLS on a cloud instance).
Closing in favour of #2634 |
No description provided.