Repository navigation
Conversation
rustbot
added
needs-fcp
This comment has been minimized.
This comment has been minimized.
ed4a0e3 to
4a42c4e
Compare
|
No changes for 95bcde1 |
CommanderStorm
added
the
S-waiting-on-community-reviews
| /// ``` | ||
| #[clippy::version = "1.100.0"] | ||
| pub REF_STRING_FROM_INSTEAD_OF_STR, | ||
| pedantic, |
There was a problem hiding this comment.
not allocating is perf unless you checked godbold and the code is identical.
Or am I missing something? 🤔
| pedantic, | |
| perf, |
There was a problem hiding this comment.
No, you are right. I just was not sure what level to put
| _ => (&[], &[], None), | ||
| }; | ||
|
|
||
| let Some(body_owner) = body_owner else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
just return here instead of the third being an Option<T>..
| && args.len() == 1 | ||
| && let ExprKind::Lit(ref lit) = args[0].kind | ||
| && let LitKind::Str(s, _) = lit.node |
There was a problem hiding this comment.
as a general rule, please move the structural pattern matching before the queries for linting perf ^^
Also, use structural pattern matching for getting the first arg
| } | ||
| } | ||
|
|
||
| fn get_literal_from_ref_string_from(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<Symbol> { |
There was a problem hiding this comment.
Please dedup this and the next function. Feels duplicated xD
There was a problem hiding this comment.
Yes, my intention was to separate the extraction of different patterns, but now that I deduplicated it.. it did not make much sense, thank you
| } | ||
| } | ||
|
|
||
| fn main() {} |
There was a problem hiding this comment.
please make this into an mod main and move everything above inside it
| { | ||
| let mut s = String::from("hello"); | ||
| s.push_str(&String::from(" world")); | ||
| //~^ ERROR: this creates a needless String allocation, use &str instead |
There was a problem hiding this comment.
instead of the error message, please assert the lint.
Error messages change over time and this is otherwise a bit of a chore ^^
| @@ -0,0 +1,97 @@ | |||
| #![allow( | |||
There was a problem hiding this comment.
consider this.
| #![allow( | |
| #![expect( |
There was a problem hiding this comment.
If I use expect, the .fixed file will fail because the lint dissapears:
--> tests/ui/ref_string_from_instead_of_str.fixed:2:11
|
2 | #![expect(clippy::needless_borrows_for_generic_args)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ after rustfix is applied, all errors should be gone, but weren't
|
full stderr:
error: this lint expectation is unfulfilled
--> tests/ui/ref_string_from_instead_of_str.fixed:2:11
|
LL | #![expect(clippy::needless_borrows_for_generic_args)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `-D unfulfilled-lint-expectations` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(unfulfilled_lint_expectations)]`
error: aborting due to 1 previous error
full stdout:
FAILURES:
tests/ui/ref_string_from_instead_of_str.fixed`
| { | ||
| let mut s = String::from("hello"); | ||
| s.push_str(&" world".to_string()); | ||
| //~^ ERROR: this creates a needless String allocation, use &str instead | ||
| } |
There was a problem hiding this comment.
is already suggested by other lints..
In general if the existing "generic" lints already suggest this rewrite, please ommit the lint from this one.
This applies below too in a bunch of cases
There was a problem hiding this comment.
I added them because I first catalogued the lint as pedantic I was not sure to report this to give extra reasons to the user. Thank you for pointing that those cases should be removed!
Gri-ffin
added
S-waiting-on-author
4a42c4e to
042308b
Compare
This comment has been minimized.
This comment has been minimized.
changelog: New lint [`ref_string_from_instead_of_str`]: use &str instead of allocating String.
042308b to
95bcde1
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
☔ The latest upstream changes (possibly #13598) made this pull request unmergeable. Please resolve the merge conflicts. |
This branch has not been deployed
5 participants
Fixes #3073
changelog: New lint [
ref_string_from_instead_of_str]: use &str instead of allocating String.r? @blyxyas