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

[missing_const_for_fn]: fix suggestions for fn with abi that requires const_extern_fn feature #13037

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

J-ZhengLi
Copy link
Member

closes: #13008


changelog: [missing_const_for_fn]: fix suggestions for fn with abi that requires const_extern_fn feature.

@rustbot
Copy link
Collaborator

rustbot commented Jul 4, 2024

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 4, 2024
Comment on lines +186 to +191
extern "C-unwind" fn c_unwind() {}
extern "system" fn system() {}
extern "system-unwind" fn system_unwind() {}
Copy link
Member Author

@J-ZhengLi J-ZhengLi Jul 4, 2024

Choose a reason for hiding this comment

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

I'm a little bit unsure should we report these with help messages like "if your are on nightly toolchain, enable const_extern_fn feature with #![feature(const_extern_fn)]" instead of just skip them, it surely will make some noise tho.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you really want to add that and can detect that a nightly toolchain is a build requirement (for example via a toolchain file), I'm not opposed to it.

Copy link
Member Author

@J-ZhengLi J-ZhengLi Jul 5, 2024

Choose a reason for hiding this comment

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

Oh, that reminds me I probably (???) could use this cx.sess().is_nightly_build to check that but I assume it will be useless in our test cases, since it will be true in all of them...

Well, I'll keep this in mind for future reference, maybe we could have some attribute like #[clippy::toolchain = "nightly"] in the future if that function turns out to be useful, but for now this might be fine (I think)~

Copy link
Contributor

Choose a reason for hiding this comment

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

For the record, the problem with is_nightly_build is that it's true when compiled on nightly. This is the wrong criteria, the code might still accept compiling on stable.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh right... I see, that'll made the users having to force nightly toolchain if they only follow the suggestion once...

hmmm, I understand why you mentioned "via a toolchain file" now, as it seems like the only sensible way here. But doing so would require clippy to have some basic de-serializer written to match that file, and I saw there's a security flaw with that file on this rustup issue, so I'm expecting rustup to make some changes regarding that file, meaning that we'll have to change them too~

For these reasons I think it might not worth the efforts, and lets just call this perfect imperfection :D

@llogiq
Copy link
Contributor

llogiq commented Jul 4, 2024

This already looks merge-worthy. So r=me unless you want to change something.

@bors delegate+

@bors
Copy link
Contributor

bors commented Jul 4, 2024

✌️ @J-ZhengLi, you can now approve this pull request!

If @llogiq told you to "r=me" after making some further change, please make that change, then do @bors r=@llogiq

@J-ZhengLi
Copy link
Member Author

@bors r=@llogiq

@bors
Copy link
Contributor

bors commented Jul 5, 2024

📌 Commit 3a71223 has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 5, 2024

⌛ Testing commit 3a71223 with merge 0aac16e...

@bors
Copy link
Contributor

bors commented Jul 5, 2024

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

@bors bors merged commit 0aac16e into rust-lang:master Jul 5, 2024
8 checks passed
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.

missing_const_for_fn triggers for C-unwind
4 participants