Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Lint push_str with a single-character string literal #5881

Merged
merged 3 commits into from
Aug 16, 2020
Merged

Lint push_str with a single-character string literal #5881

merged 3 commits into from
Aug 16, 2020

Conversation

wiomoc
Copy link
Contributor

@wiomoc wiomoc commented Aug 9, 2020 •
edited
Loading

Fixes #5875
changelog: * [single_char_push_str]

@rust-highfive
Copy link

r? @phansch

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 9, 2020
@wiomoc wiomoc force-pushed the feature/single-char-push_str branch from 5dccf12 to 260e9ed Compare August 9, 2020 16:02
@flip1995
Copy link
Member

r? @flip1995

r? @ebroto Can you review this PR please?

@rust-highfive rust-highfive assigned flip1995 and unassigned phansch Aug 12, 2020
ebroto
ebroto suggested changes Aug 12, 2020
View reviewed changes
Copy link
Member

@ebroto ebroto left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some thoughts.

As a nit, in the changelog line the lint should be inside backticks so that the link to the description works from the changelog.

cc @flip1995

clippy_lints/src/single_char_push_str.rs Outdated Show resolved Hide resolved
clippy_lints/src/single_char_push_str.rs Outdated
/// string.push('R');
/// ```
pub SINGLE_CHAR_PUSH_STR,
style,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is correct. The issue mentioned better performance, but this does not seem to be true as the generated assembler is exactly the same. See here and here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like rustc is optimizing the push_str("R") to a push('R') so it's not a performance difference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I was not precise enough, I meant that Clippy's suggestion would not improve the performance.

clippy_lints/src/single_char_push_str.rs Outdated Show resolved Hide resolved
clippy_lints/src/single_char_push_str.rs Outdated
if let Some(fn_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
if match_def_path(cx, fn_def_id, &paths::PUSH_STR);
if let ExprKind::Lit(ref lit) = extension_string.kind;
if let LitKind::Str(symbol,_) = lit.node;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should ignore raw strings as the intention of the user is probably to avoid escaping the character.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The single_char_pattern does replace raw strings, should i remove that feature

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I was not aware of that lint. I think we should not change the behavior at this point.

clippy_lints/src/single_char_push_str.rs Outdated Show resolved Hide resolved
tests/ui/single_char_push_str.fixed Show resolved Hide resolved
@flip1995
Copy link
Member

What I just realized: Wouldn't it be easier/better to enhance the single_char_pattern lint, instead of implementing a new lint?

@wiomoc wiomoc force-pushed the feature/single-char-push_str branch from 10fa7b3 to 783108d Compare August 14, 2020 10:55
ebroto
ebroto suggested changes Aug 14, 2020
View reviewed changes
Copy link
Member

@ebroto ebroto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just some really minor nits

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
@wiomoc wiomoc force-pushed the feature/single-char-push_str branch from 783108d to b381ade Compare August 14, 2020 23:44
@ebroto
Copy link
Member

ebroto commented Aug 15, 2020 •
edited
Loading

LGTM, cc @flip1995

Thanks!

@flip1995
Copy link
Member

Thanks!

@bors r=ebroto,flip1995

Thanks for the review @ebroto

@bors
Copy link
Collaborator

bors commented Aug 16, 2020

📌 Commit b381ade has been approved by ebroto,flip1995

@bors
Copy link
Collaborator

bors commented Aug 16, 2020

⌛ Testing commit b381ade with merge c8e05fc...

@bors
Copy link
Collaborator

bors commented Aug 16, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: ebroto,flip1995
Pushing c8e05fc to master...

@bors bors merged commit c8e05fc into rust-lang:master Aug 16, 2020
@bors bors mentioned this pull request Aug 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebroto ebroto ebroto requested changes

@flip1995 flip1995 flip1995 left review comments

@camelid camelid camelid left review comments

Assignees

@flip1995 flip1995

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Warn on push_str with a single-character string literal
7 participants
@wiomoc @rust-highfive @flip1995 @ebroto @bors @camelid @phansch