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

[significant_drop_tightening] Add MVP #10163

Merged
merged 2 commits into from
Feb 16, 2023
Merged

[significant_drop_tightening] Add MVP #10163

merged 2 commits into from
Feb 16, 2023

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Jan 5, 2023 •
edited
Loading

cc #9399

Creates the lint with minimum functionalities, which is a good start IMO.

changelog: new lint: [significant_drop_tightening]
#10163

@rustbot
Copy link
Collaborator

rustbot commented Jan 5, 2023

r? @llogiq

(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 5, 2023
@llogiq
Copy link
Contributor

llogiq commented Jan 5, 2023

Feel free to add the tests and implementation. Then I'll try and have a review within the next 7 days or possibly sooner, so we don't need to carry incomplete code through.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jan 5, 2023

@llogiq If you have the bandwidth to review a full PR, then I will reach back to you in the following weeks with a more mature implementation.

@c410-f3r c410-f3r changed the title [early_drop_lock] Add initial skeleton [early_lock_drop] Add initial skeleton Jan 6, 2023
@llogiq
Copy link
Contributor

llogiq commented Jan 19, 2023

@rustbot author

@rustbot rustbot 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 Jan 19, 2023
@c410-f3r c410-f3r changed the title [early_lock_drop] Add initial skeleton [early_lock_drop] Add MVP Jan 23, 2023
@c410-f3r c410-f3r changed the title [early_lock_drop] Add MVP [significant_drop_tightening] Add MVP Feb 7, 2023
c410-f3r
c410-f3r commented Feb 7, 2023
View reviewed changes
Copy link
Contributor Author

@c410-f3r c410-f3r left a comment

Choose a reason for hiding this comment

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

@llogiq The lint is now in a minimal functional state for experimentation in the sense that it will detect and suggest some situations were a path/bind/variable marked with #[clippy::has_significant_drop] is being dropped at the end of its encapsulated block.

Pretty much as arithmetic_side_effects; rough edges, bugs and enhancements will be figured out in a sequence of future subsequent PRs.

clippy_lints/src/sig_drop_aux.rs Outdated Show resolved Hide resolved
clippy_lints/src/significant_drop_tightening.rs Show resolved Hide resolved
tests/ui/significant_drop_tightening.rs Outdated Show resolved Hide resolved
@c410-f3r
Copy link
Contributor Author

c410-f3r commented Feb 7, 2023

cargo dev fmt is locally panicking 🤷

Formatting check failed.
Run `cargo dev fmt` to update formatting.

@c410-f3r c410-f3r force-pushed the lock-1 branch 4 times, most recently from a746638 to 7e1f6bf Compare February 9, 2023 17:35
@c410-f3r
Copy link
Contributor Author

c410-f3r commented Feb 9, 2023 •
edited
Loading

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Feb 9, 2023
llogiq
llogiq approved these changes Feb 10, 2023
View reviewed changes
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

Thank you! In general, this looks good. It certainly meets our standards for a nursery lint. I have but a few niggles on code style, please address them, and then I'll gladly r+.

clippy_lints/src/significant_drop_tightening.rs Show resolved Hide resolved
clippy_lints/src/significant_drop_tightening.rs Outdated Show resolved Hide resolved
clippy_lints/src/significant_drop_tightening.rs Outdated Show resolved Hide resolved
clippy_lints/src/significant_drop_tightening.rs Outdated Show resolved Hide resolved
clippy_lints/src/significant_drop_tightening.rs Outdated Show resolved Hide resolved
clippy_lints/src/significant_drop_tightening.rs Outdated Show resolved Hide resolved
@llogiq
Copy link
Contributor

llogiq commented Feb 12, 2023

One final nit, then I'll be happy to have bors merge this.

@llogiq
Copy link
Contributor

llogiq commented Feb 16, 2023

Thank you!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 16, 2023

📌 Commit 1b286b1 has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 16, 2023

⌛ Testing commit 1b286b1 with merge 4369a67...

@bors
Copy link
Contributor

bors commented Feb 16, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 4369a67 to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Feb 16, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 4369a67 to master...

@bors bors merged commit 4369a67 into rust-lang:master Feb 16, 2023
@c410-f3r
Copy link
Contributor Author

Thanks @llogiq

@bors bors mentioned this pull request Feb 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@llogiq llogiq llogiq approved these changes

Assignees

@llogiq llogiq

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
@c410-f3r @rustbot @llogiq @bors