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

Add new check for passing pointers to an asm! block with nomem option #13247

Merged
merged 1 commit into from
Sep 3, 2024
Merged

Add new check for passing pointers to an asm! block with nomem option #13247

merged 1 commit into from
Sep 3, 2024

Conversation

Soveu
Copy link
Contributor

@Soveu Soveu commented Aug 9, 2024

changelog: Add new check for passing pointers to an asm! block with nomem option

Continuing work from rust-lang/rust#127063

@rustbot
Copy link
Collaborator

rustbot commented Aug 9, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @y21 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 9, 2024
y21
y21 reviewed Aug 10, 2024
View reviewed changes
Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

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

This looks like a useful lint to have. Implementation generally looks good to me aside from a few small things 👍

clippy_lints/src/pointer_in_nomem_asm_block.rs Outdated Show resolved Hide resolved
clippy_lints/src/pointer_in_nomem_asm_block.rs Outdated Show resolved Hide resolved
clippy_lints/src/pointer_in_nomem_asm_block.rs Outdated Show resolved Hide resolved
clippy_lints/src/pointer_in_nomem_asm_block.rs Outdated Show resolved Hide resolved
tests/ui/pointer_in_nomem_asm_block.rs Outdated Show resolved Hide resolved
tests/ui/pointer_in_nomem_asm_block.rs Outdated Show resolved Hide resolved
@Soveu
Copy link
Contributor Author

Soveu commented Aug 11, 2024

I think all the comments were addressed

This looks like a useful lint to have.

Yeah, after spending two days to debug this issue, certainly 😅

y21
y21 approved these changes Aug 18, 2024
View reviewed changes
Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

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

Two small nits/typos then this should be good

clippy_lints/src/pointer_in_nomem_asm_block.rs Outdated Show resolved Hide resolved
clippy_lints/src/pointer_in_nomem_asm_block.rs Outdated Show resolved Hide resolved
@y21
Copy link
Member

y21 commented Aug 18, 2024

Also went ahead and opened the FCP thread on zulip that we require for new lints: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/FCP.3A.20pointer_in_nomem_asm_block

@Soveu
Copy link
Contributor Author

Soveu commented Aug 24, 2024

LGTM :)

@y21 y21 added S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Aug 24, 2024
@bors
Copy link
Contributor

bors commented Aug 28, 2024

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

@y21
Copy link
Member

y21 commented Sep 1, 2024

The FCP thread has been up for about two weeks and has received some 👍 reactions and no concerns, so I'd say we're good. Just needs a rebase now. Can you also squash the commits while you're at it?

@y21 y21 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) labels Sep 1, 2024
@Soveu
Copy link
Contributor Author

Soveu commented Sep 3, 2024

@y21 done

@y21
Copy link
Member

y21 commented Sep 3, 2024

Thanks for implementing this lint!

@bors r+

@bors
Copy link
Contributor

bors commented Sep 3, 2024

📌 Commit 273b561 has been approved by y21

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 3, 2024

⌛ Testing commit 273b561 with merge e8ba5d1...

@bors
Copy link
Contributor

bors commented Sep 3, 2024

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

@bors bors merged commit e8ba5d1 into rust-lang:master Sep 3, 2024
11 checks passed
@Soveu
Copy link
Contributor Author

Soveu commented Sep 3, 2024

🎉

This was referenced Sep 3, 2024
Merged
Open
Merged
Merged
Open
@bors bors mentioned this pull request Sep 3, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@y21 y21 y21 approved these changes

Assignees

@y21 y21

Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Soveu @rustbot @y21 @bors