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

[slow_vector_initialization]: catch Vec::new() followed by .resize(len, 0) #11198

Merged
merged 3 commits into from
Jul 25, 2023

Conversation

y21
Copy link
Member

@y21 y21 commented Jul 20, 2023

Closes #10938

changelog: [slow_vector_initialization]: catch Vec::new() followed by .resize(len, 0)

@rustbot
Copy link
Collaborator

rustbot commented Jul 20, 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 Jul 20, 2023
@Centri3
Copy link
Member

Centri3 commented Jul 24, 2023

As requested by Manishearth on zulip,

r? @Centri3

@rustbot rustbot assigned Centri3 and unassigned Manishearth Jul 24, 2023
Copy link
Member

@Centri3 Centri3 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! Just a couple nits and I think this is good to go

clippy_lints/src/slow_vector_initialization.rs Outdated Show resolved Hide resolved
clippy_lints/src/slow_vector_initialization.rs Outdated Show resolved Hide resolved
clippy_lints/src/slow_vector_initialization.rs Outdated Show resolved Hide resolved
.size_expr
.expect("length expression must be set by this point"),
"len",
);

span_lint_and_then(cx, SLOW_VECTOR_INITIALIZATION, slow_fill.span, msg, |diag| {
Copy link
Member

Choose a reason for hiding this comment

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

Not part of your PR, but if this could also suggest to remove the following call that'd be nice. Not sure if that'd have adverse side effects though

(should probably also be a separate PR ^^)

@Centri3
Copy link
Member

Centri3 commented Jul 25, 2023

Thanks! @bors r+

@bors
Copy link
Collaborator

bors commented Jul 25, 2023

📌 Commit c0484b7 has been approved by Centri3

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jul 25, 2023

⌛ Testing commit c0484b7 with merge 70c5798...

@bors
Copy link
Collaborator

bors commented Jul 25, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Centri3
Pushing 70c5798 to master...

@bors bors merged commit 70c5798 into rust-lang:master Jul 25, 2023
4 checks passed
@bors bors mentioned this pull request Jul 25, 2023
bors added a commit that referenced this pull request Aug 9, 2023
…,djc

[`slow_vector_initialization`]: clarify why `Vec::new()` + resize is worse

#11198 extended this lint to also warn on `Vec::new()` + `resize(0, len)`, but did not update the lint documentation, so it left some confused (#10938 (comment)).
This PR should make it a bit more clear. (cc `@djc` `@vi` what do you think about this?)

<details>
<summary>More details</summary>

Godbolt for `Vec::new()` + `.resize(x, 0)`: https://godbolt.org/z/e7q9xc9rG

The resize call first does a normal allocation (`__rust_alloc`):
```asm
alloc::raw_vec::finish_grow:
  ...
  cmp     qword ptr [rcx + 8], 0
  je      .LBB1_7  ; if capacity == 0 -> LBB1_7

.LBB1_7:
  ...
  call    qword ptr [rip + __rust_alloc@GOTPCREL]
```

*Then* a memset for zero initialization:
```asm
example::f:
  ...
  xor     esi, esi  ; 0
  call    qword ptr [rip + memset@GOTPCREL]
```
------------

Godbolt for `vec![0; len]`: https://godbolt.org/z/M3vr53vWY

Important bit:
```asm
example::f:
  ...
  call    qword ptr [rip + __rust_alloc_zeroed@GOTPCREL]
```

</details>

changelog: [`slow_vector_initialization`]: clarify why `Vec::new()` + resize is worse than `vec![0; len]`
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.

slow-vector-initialization should catch Vec::new followed by Vec::resize(len, 0).
5 participants