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

Add unnecessary_reserve lint #10157

Closed
wants to merge 21 commits into from
Closed

Conversation

chansuke
Copy link
Contributor

@chansuke chansuke commented Jan 4, 2023

This PR has taken over from #9073.

Fixes #8982.


changelog: new lint: [unnecessary_reserve]
#10157

@rustbot
Copy link
Collaborator

rustbot commented Jan 4, 2023

r? @Manishearth

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 4, 2023
@Manishearth
Copy link
Member

r? @flip1995 since you were reviewing the previous one

@rustbot rustbot assigned flip1995 and unassigned Manishearth Jan 4, 2023
@bors
Copy link
Contributor

bors commented Jan 7, 2023

☔ The latest upstream changes (presumably #10164) made this pull request unmergeable. Please resolve the merge conflicts.

@chansuke
Copy link
Contributor Author

chansuke commented Jan 7, 2023

Ah, sorry,this is still WIP

@chansuke chansuke changed the title Add unnecessary_reserve lint WIP:Add unnecessary_reserve lint Jan 7, 2023
@chansuke chansuke changed the title WIP:Add unnecessary_reserve lint Add unnecessary_reserve lint Jan 7, 2023
@chansuke chansuke marked this pull request as ready for review January 7, 2023 14:02
@bors
Copy link
Contributor

bors commented Jan 19, 2023

☔ The latest upstream changes (presumably #10206) made this pull request unmergeable. Please resolve the merge conflicts.

@chansuke chansuke force-pushed the issue_8982 branch 2 times, most recently from 14ca695 to 3f17aa4 Compare January 24, 2023 18:25
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Mostly style issues and one test seems to be missing.

README.md Outdated
@@ -5,7 +5,7 @@

A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.

[There are over 550 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
[There are over 600 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
Copy link
Member

Choose a reason for hiding this comment

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

🎉 🎉 🎉 🎉 🎉

@@ -603,6 +603,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::unnamed_address::FN_ADDRESS_COMPARISONS_INFO,
crate::unnamed_address::VTABLE_ADDRESS_COMPARISONS_INFO,
crate::unnecessary_owned_empty_strings::UNNECESSARY_OWNED_EMPTY_STRINGS_INFO,
crate::unnecessary_reserve::UNNECESSARY_RESERVE_INFO,
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be

Suggested change
crate::unnecessary_reserve::UNNECESSARY_RESERVE_INFO,
crate::unnecessary_reserve::UNNECESSARY_RESERVE,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@flip1995 Thank you so much for your review. UNNECESSARY_RESERVE is defined in clippy_lints/src/unnecessary_reserve.rs as impl_lint_pass!(UnnecessaryReserve => [UNNECESSARY_RESERVE]); which is a LintPass and thought that need to declare as an another constants.


impl<'tcx> LateLintPass<'tcx> for UnnecessaryReserve {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
if !self.msrv.meets(msrvs::CHECK_UNNECESSARY_RESERVE) {
Copy link
Member

Choose a reason for hiding this comment

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

Usually we name the MSRV constant after the feature. I don't know if this has a feature name. If not, I would call it EXTEND_IMPLICIT_RESERVE or something along those lines.

{
read_found = true;
}
let _ = !read_found;
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, trying to flip read_found. I will refactor this

Comment on lines +154 to +159
pub const VEC_RESERVE: [&str; 4] = ["alloc", "vec", "Vec", "reserve"];
pub const VEC_DEQUE_RESERVE: [&str; 5] = ["alloc", "collections", "vec_deque", "VecDeque", "reserve"];
Copy link
Member

Choose a reason for hiding this comment

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

Those are a lot of paths added here. It may be worth opening a rust-lang/rust PR adding diagnostic items for those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@flip1995 Thank you so much for your review and suggestion. I will fix these.

Comment on lines +18 to +20
// do not lint
vec.reserve(1);
vec.extend([1]);
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 one test that is missing is something like

vec.reserve(array1.len());
vec.extend(array2);

This might be covered by this, but I'm not 100% sure.

This should not be linted. Doesn't make much sense to write something like this, but it should be an easy FP to avoid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

@bors
Copy link
Contributor

bors commented Feb 3, 2023

☔ The latest upstream changes (presumably #10028) made this pull request unmergeable. Please resolve the merge conflicts.

@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Feb 3, 2023
@bors
Copy link
Contributor

bors commented Feb 23, 2023

☔ The latest upstream changes (presumably #10392) made this pull request unmergeable. Please resolve the merge conflicts.

@chansuke chansuke force-pushed the issue_8982 branch 2 times, most recently from f8b338f to c8f604a Compare February 25, 2023 09:46
@chansuke chansuke requested a review from flip1995 February 25, 2023 20:49
@bors
Copy link
Contributor

bors commented Mar 7, 2023

☔ The latest upstream changes (presumably #10415) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Apr 23, 2023

☔ The latest upstream changes (presumably #10578) made this pull request unmergeable. Please resolve the merge conflicts.

@blyxyas
Copy link
Member

blyxyas commented Oct 7, 2023

@chansuke I'm closing this for inactivity (don't worry, it isn't anyone's fault! Just that Philipp doesn't have time to review anymore ฅ^•ﻌ•^ฅ).

Feel free to re-open this if you're still interested in developing in this branch. But I'd advice you to create a new branch and a new PR.

If you are still interested in this and create another PR, write r? @blyxyas somewhere in the PR body so that it gets assigned to me!

@blyxyas blyxyas closed this Oct 7, 2023
@flip1995 flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive-closed Status: Closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unnecessary Vec/VecDeque reserve lint
7 participants