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.

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

Fix false positive for needless_character_iteration lint #12886

Merged

Conversation

GuillaumeGomez
Copy link
Member

Fixes #12879.

changelog: Fix false positive for needless_character_iteration lint

@rustbot
Copy link
Collaborator

rustbot commented Jun 3, 2024

r? @blyxyas

rustbot has assigned @blyxyas.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 3, 2024
@GuillaumeGomez GuillaumeGomez force-pushed the fix-needless_character_iteration branch from cc46063 to 312c7f3 Compare June 3, 2024 19:59
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

Just some optimization, and this should be ready!

ExprKind::Call(fn_path, [arg]) => {
if let ExprKind::Path(path) = fn_path.kind
&& let Some(fn_def_id) = cx.qpath_res(&path, fn_path.hir_id).opt_def_id()
&& match_def_path(cx, fn_def_id, &["core", "char", "methods", "<impl char>", "is_ascii"])
&& path_to_local_id(peels_expr_ref(arg), first_param)
&& let Some(snippet) = snippet_opt(cx, before_chars)
// If we have `!is_ascii`, then only `.any()` should warn. And if the condition is
// `is_ascii`, then only `.all()` should warn.
&& revert != is_all
Copy link
Member

Choose a reason for hiding this comment

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

Could you lift this to avoid running the expensive conditions unnecessarily? (Same with the other condition)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point!

@GuillaumeGomez GuillaumeGomez force-pushed the fix-needless_character_iteration branch from 312c7f3 to 158b658 Compare June 4, 2024 19:12
@GuillaumeGomez
Copy link
Member Author

Moved the condition sooner for better performance as suggested.

Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! ❤️

@blyxyas
Copy link
Member

blyxyas commented Jun 5, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jun 5, 2024

📌 Commit 158b658 has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 5, 2024

⌛ Testing commit 158b658 with merge 10d1f32...

@bors
Copy link
Contributor

bors commented Jun 5, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 10d1f32 to master...

@bors bors merged commit 10d1f32 into rust-lang:master Jun 5, 2024
5 checks passed
@GuillaumeGomez GuillaumeGomez deleted the fix-needless_character_iteration branch June 6, 2024 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

needless_character_iteration false positive
4 participants