Repository navigation
Moved lints from strings to methods - #17512
Conversation
rustbot
added
S-waiting-on-review
|
Can you split this into one commit per lint moved, please. Also when fixing CI errors you're better off force-pushing. We're going to ask you to squash those commits before merging and it doesn't affect reviewers much if you're immediately changing commits. |
04a75b8 to
27e5b3f
Compare
This comment has been minimized.
This comment has been minimized.
27e5b3f to
e28d0b2
Compare
This comment has been minimized.
This comment has been minimized.
| (sym::split_whitespace, []) => { | ||
| trim_split_white_space::check(cx, expr, recv, call_span); | ||
| }, |
There was a problem hiding this comment.
This is a fix rather than an internal change because of the macro guard you pointed out in the PR description. If necessary, you can split the commit of TRIM_SPLIT_WHITESPACE into two:
- Add macro guard in
string.rs(fix) - Move it to
methods/mod.rs(internal change)
I'm not sure whether combining them into single fix is conventional.
rustbot
removed
the
S-waiting-on-community-reviews
|
r? @blyxyas rustbot has assigned @blyxyas for the project review. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| } | ||
|
|
||
| if let ExprKind::MethodCall(path, receiver, ..) = &e.kind | ||
| if !e.span.in_external_macro(cx.sess().source_map()) |
There was a problem hiding this comment.
Why did you move this external macro call up? It's more expensive than checking if we're dealing with a method call.
It should be kept at the same place ^ↀᴥↀ^
There was a problem hiding this comment.
You are completely right. I didn't want to move it in the first place; I probably moved it unintentionally while messing around with things :)
Should be fixed now
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
e28d0b2 to
258c8ee
Compare
rustbot
added
S-waiting-on-review
There was a problem hiding this comment.
LGTM, thanks for addressing this issue! ❤️ ฅ•ω•ฅ
Could you squash the commits @MassimilianoBaglioni
This comment has been minimized.
This comment has been minimized.
258c8ee to
963a177
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. |
rustbot
removed
the
S-waiting-on-review
changelog: none
Moves
STRING_FROM_UTF8_AS_BYTESandTRIM_SPLIT_WHITESPACEfromstrings.rsinto the sharedMethodsdispatch inmethods.rs.Questions:
TRIM_SPLIT_WHITESPACEnow inherits thefrom_expansion()check inMethods::check_expr. It had no macro guard before. Tests still pass (no test covers macro-expanded call sites), but this is an untested behavior change worth flagging.STRING_LIT_AS_BYTEStoo, but some tests use macros, and thefrom_expansion()check inMethods::check_exprprevents the lint from firing on those cases, so tests failed. I kept it instrings.rsfor now because of this. How should I proceed?STR_TO_STRINGnot moved yet, one branch matchesExprKind::Path, notMethodCall, so it doesn't fit the shared dispatch. Should this get its own arm incheck_expr, stay separate, or something else?Also happy to get feedback/comments on the code already moved, not just on the open questions above.
fixes #14253