Skip to content

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

Merged
bors merged 1 commit into
rust-lang:masterfrom
Soveu:sus-asm-options
Sep 3, 2024
Merged

Add new check for passing pointers to an asm! block with nomem option#13247
bors merged 1 commit into
rust-lang:masterfrom
Soveu:sus-asm-options

Conversation

@Soveu

@Soveu Soveu commented Aug 9, 2024

Copy link
Copy Markdown
Contributor

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

Continuing work from rust-lang/rust#127063

@rustbot

rustbot commented Aug 9, 2024

Copy link
Copy Markdown
Collaborator

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

@y21 y21 left a comment

Copy link
Copy Markdown
Member

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 👍

Comment thread clippy_lints/src/pointer_in_nomem_asm_block.rs Outdated
Comment thread clippy_lints/src/pointer_in_nomem_asm_block.rs Outdated
Comment thread clippy_lints/src/pointer_in_nomem_asm_block.rs Outdated
Comment thread clippy_lints/src/pointer_in_nomem_asm_block.rs
Comment thread tests/ui/pointer_in_nomem_asm_block.rs Outdated
Comment thread tests/ui/pointer_in_nomem_asm_block.rs
@Soveu

Soveu commented Aug 11, 2024

Copy link
Copy Markdown
Contributor Author

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

@y21 y21 left a comment

Copy link
Copy Markdown
Member

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

Comment thread clippy_lints/src/pointer_in_nomem_asm_block.rs Outdated
Comment thread clippy_lints/src/pointer_in_nomem_asm_block.rs Outdated
@y21

y21 commented Aug 18, 2024

Copy link
Copy Markdown
Member

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

Soveu commented Aug 24, 2024

Copy link
Copy Markdown
Contributor Author

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

bors commented Aug 28, 2024

Copy link
Copy Markdown
Contributor

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

@y21

y21 commented Sep 1, 2024

Copy link
Copy Markdown
Member

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

Soveu commented Sep 3, 2024

Copy link
Copy Markdown
Contributor Author

@y21 done

@y21

y21 commented Sep 3, 2024

Copy link
Copy Markdown
Member

Thanks for implementing this lint!

@bors r+

@bors

bors commented Sep 3, 2024

Copy link
Copy Markdown
Contributor

📌 Commit 273b561 has been approved by y21

It is now in the queue for this repository.

@bors

bors commented Sep 3, 2024

Copy link
Copy Markdown
Contributor

⌛ Testing commit 273b561 with merge e8ba5d1...

@bors

bors commented Sep 3, 2024

Copy link
Copy Markdown
Contributor

☀️ 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
@Soveu

Soveu commented Sep 3, 2024

Copy link
Copy Markdown
Contributor Author

🎉

This was referenced Sep 3, 2024
Merged
Closed
Merged
Merged
Closed
@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