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

new lint: large_stack_frames #10827

Merged
merged 8 commits into from
Jun 12, 2023
Merged

new lint: large_stack_frames #10827

merged 8 commits into from
Jun 12, 2023

Conversation

y21
Copy link
Member

@y21 y21 commented May 25, 2023

This implements a lint that looks for functions that use a lot of stack space.

It uses the MIR because conveniently every temporary gets its own local and I think it maps best to stack space used in a function.
It's probably going to be quite inaccurate in release builds, but at least for debug builds where opts are less aggressive on LLVM's side I think this is accurate "enough".

(This does not work for generic functions yet. Not sure if I should try to get it working in this PR or if it could land without it for now and be added in a followup PR.)

I also put it under the nursery category because this probably needs more work...

changelog: new lint: [large_stack_frames]

@rustbot
Copy link
Collaborator

rustbot commented May 25, 2023

r? @dswij

(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 May 25, 2023
dswij
dswij reviewed Jun 7, 2023
View reviewed changes
Copy link
Member

@dswij dswij left a comment

Choose a reason for hiding this comment

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

Just a small comment

clippy_lints/src/large_stack_frames.rs Outdated Show resolved Hide resolved
dswij
dswij approved these changes Jun 8, 2023
View reviewed changes
Copy link
Member

@dswij dswij 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.

LGTM, thanks for the PR and the detailed description of the lint!

Seems like there's a conflict, can you help to rebase? I think it's good to merge afterward

@y21 y21 force-pushed the large-stack-frames branch from b7edba3 to 617e504 Compare June 8, 2023 11:07
@y21
Copy link
Member Author

y21 commented Jun 8, 2023

ok, I rebased and also made the lint description a tiny bit easier to read (got rid of all the references to "MIR")

@dswij
Copy link
Member

dswij commented Jun 10, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jun 10, 2023

📌 Commit 617e504 has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 10, 2023

⌛ Testing commit 617e504 with merge c0e3933...

bors added a commit that referenced this pull request Jun 10, 2023
@bors
Auto merge of #10827 - y21:large-stack-frames, r=dswij
c0e3933 
new lint: `large_stack_frames`

This implements a lint that looks for functions that use a lot of stack space.

It uses the MIR because conveniently every temporary gets its own local and I think it maps best to stack space used in a function.
It's probably going to be quite inaccurate in release builds, but at least for debug builds where opts are less aggressive on LLVM's side I think this is accurate "enough".

(This does not work for generic functions yet. Not sure if I should try to get it working in this PR or if it could land without it for now and be added in a followup PR.)

I also put it under the nursery category because this probably needs more work...

changelog: new lint: [`large_stack_frames`]
@bors
Copy link
Contributor

bors commented Jun 10, 2023

💔 Test failed - checks-action_test

@dswij
Copy link
Member

dswij commented Jun 10, 2023

@y21 Seems like you need to update lint_configuration.md by running cargo collect-metadata

https://github.com/rust-lang/rust-clippy/actions/runs/5230370766/jobs/9443890954

@y21
Copy link
Member Author

y21 commented Jun 11, 2023

sorry, I wasn't home for most of the weekend. Updated lint_configuration.md now. I'm not sure why the file was broken in the first place to cause this error (I did run cargo collect-metadata before), might've been a bad rebase 😅

@bors
Copy link
Contributor

bors commented Jun 12, 2023

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

@y21 y21 force-pushed the large-stack-frames branch from 1708196 to 6ad7c6f Compare June 12, 2023 10:31
@dswij
Copy link
Member

dswij commented Jun 12, 2023

Thanks! Let's try that again 😄

@bors r+

@bors
Copy link
Contributor

bors commented Jun 12, 2023

📌 Commit 6ad7c6f has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 12, 2023

⌛ Testing commit 6ad7c6f with merge ebafbfc...

@bors
Copy link
Contributor

bors commented Jun 12, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij
Pushing ebafbfc to master...

@bors bors merged commit ebafbfc into rust-lang:master Jun 12, 2023
This was referenced Jun 12, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dswij dswij dswij approved these changes

Assignees

@dswij dswij

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.
4 participants
@y21 @rustbot @dswij @bors