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

new lint: vec.drain(..) instead of vec.clear() #9339

Closed
philpax opened this issue Aug 16, 2022 · 12 comments · Fixed by #10528
Closed

new lint: vec.drain(..) instead of vec.clear() #9339

philpax opened this issue Aug 16, 2022 · 12 comments · Fixed by #10528
Assignees
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy

Comments

@philpax
Copy link

philpax commented Aug 16, 2022

What it does

Checks for usage of vec.drain(..) (iterator dropped immediately) which can be replaced with vec.clear()

Lint Name

unnecessary_vec_drain

Category

style

Advantage

  • Makes intent clearer
  • Marginally more efficient, especially in debug builds

Drawbacks

No response

Example

vec.drain(..);

Could be written as:

vec.clear();
@philpax philpax added the A-lint Area: New lints label Aug 16, 2022
@Jarcho Jarcho added the good-first-issue These issues are a good way to get started with Clippy label Aug 17, 2022
@carloosaf
Copy link

@rustbot claim

@carloosaf carloosaf removed their assignment Aug 22, 2022
@koka831
Copy link
Contributor

koka831 commented Aug 22, 2022

@rustbot claim

@koka831 koka831 removed their assignment Sep 7, 2022
@lana99
Copy link

lana99 commented Sep 7, 2022

@rustbot claim

@czotomo
Copy link

czotomo commented Oct 6, 2022

Hi @lana99, are you working on this or is this up for grabs?

@lana99
Copy link

lana99 commented Oct 6, 2022

I’ve been busy but I will probably push code for the issue this weekend. @czotomo

@bluthej
Copy link
Contributor

bluthej commented Mar 4, 2023

Hi @lana99, I don't know if you are still working on this but as I was looking at #9623 I was thinking that the example in the documentation you provided might be improved from this:

let mut vec: Vec<i32> = Vec::new();
vec.drain(..);

to this:

let mut v = vec![0, 1, 2];
v.drain(..);

to make it conform to the examples of the Drain documentation and the Vec documentation.

Sorry if this isn't the place to discuss this, and I don't mean to be nosy, I was just trying to learn from other people's PR how to contribute and yours caught my attention 🙂

@lana99 lana99 removed their assignment Mar 4, 2023
@lana99
Copy link

lana99 commented Mar 4, 2023

Ah great, im not working on it anymore , so its up for grabs :)

@bluthej
Copy link
Contributor

bluthej commented Mar 11, 2023

@flip1995 I would like to pick up where @lana99 left off, but first I would like suggest renaming the lint. I looked at #9623 and saw that you suggested renaming UNNECESSARY_VEC_DRAIN to NEEDLESS_VEC_DRAIN, but after thumbing through the naming guidelines and the existing lints, I came up with the following proposals (I prefer number 1 personally):

  1. clear_with_drain: It's nice because it's consistent with iter_with_drain (and also get_last_with_len) and it expresses well what the person is doing, i.e. clearing a vector with a call to drain
  2. drain_instead_of_clear: Also expresses well what the person is doing and consistent with lints like from_iter_instead_of_collect, however it's a bit less concise

My point is, while looking at existing lints it occurred to me that unnecessary_... lints tend to deal with something that should be removed entirely because it is not needed. Here, the call to drain is needed in order to clear the vector, it's just that it's not the best way to do it.

Any thoughts?

@bluthej
Copy link
Contributor

bluthej commented Mar 18, 2023

@rustbot claim

@bluthej
Copy link
Contributor

bluthej commented Mar 18, 2023

Hi @philpax, I started working on this but I'm a newbie and I have some questions :) (on the lint itself, not necessarily on how to contribute).

Do you know of anyone who might have some time to mentor me? The active members link in CONTRIBUTING.md seems to be pointing to a file that does not exist anymore, so I am to sure who I can contact.

Cheers

@philpax
Copy link
Author

philpax commented Mar 18, 2023

I'm afraid not - I'm just a user myself! I'd suggest going to the #clippy channel of the Zulip and asking there.

@bluthej
Copy link
Contributor

bluthej commented Mar 19, 2023

Oh I see! I'll do that, thank you 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants