Stabilize `const_fn_transmute`, `const_fn_union` by jhpratt · Pull Request #8576...

Copy link

Contributor

rust-highfive commented on May 28

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

rust-highfive commented on May 28

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

jhpratt commented on May 28

@rustbot label: -S-waiting-on-review +S-waiting-on-author +A-const-fn +T-compiler

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

jhpratt commented on Jun 2

Looking at the failure, I'm not really sure what should be done about this. It's wholly unrelated to this PR (it's only being run because I remove the feature gate) and the lints are objectively correct — but it's also what's being tested. I think it's an issue with the script that's running the tests: -D warnings shouldn't be passed when clippy warnings are being checked.

When running x t --stage 2 locally, everything succeeds. I use Fedora 34/x64_64-unknown-linux-gnu if that's relevant.

r? @oli-obk

RalfJung commented on Jun 2

It's wholly unrelated to this PR (it's only being run because I remove the feature gate)

The test will get run by bors either way, so even if it wouldn't be run here we'd still get the same failure later. Therefore it is definitely related to the PR, in the sense that these warnings do not show up on master, but they do show up with your PR applied.
@rust-lang/clippy what do you recommend we do with these -D warning failures in clippy tests? Somehow just stabilizing a feature makes new warnings show up...?

RalfJung commented on Jun 2

Oh wait, I misread... these ware not new warnings, the line numbers are just wrong.
Sadly I don't know how to auto---bless clippy tests, so I think you will have to adjust the line numbers in the stderr files by hand.

jhpratt commented on Jun 2

Huh, you're right. Not sure why the lines removed weren't shown by rustbot. All I saw was the additions and assumed it was an error.

flip1995 commented on Jun 2

./x.py test --bless src/tools/clippy should work.

jhpratt commented on Jun 2

Thanks @flip1995! I incidentally just tried that figuring it can't hurt and found it helped :)

flip1995 commented on Jun 2

Great! Clippy changes LGTM.

This comment has been hidden.

bors commented 10 days ago

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

jhpratt commented 10 days ago

At this point I'm going to stop playing whack-a-mole and just rebase one final time once requested. I know this PR is nominated, so I'll hold off until that discussion takes place.

RalfJung commented 10 days ago

@rust-lang/lang this is waiting on someone starting FCP :)

cramertj commented 7 days ago

@rfcbot fcp merge

Link to stabilization report
Tests are visible in the changed files of this PR.

rfcbot commented 7 days ago •

Team member @cramertj has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented 3 days ago

This is now entering its final comment period, as per the review above.

oli-obk commented 3 days ago

Hey @jhpratt, I'm assuming that this FCP will run through smoothly, so if you rebase and fix the conflicts, we can r+ right now and revert if anything comes up within the FCP.

jhpratt commented 2 days ago

Rebased and green.

oli-obk commented yesterday

@bors r+ Thanks!

bors commented yesterday

Commit c6cdf74 has been approved by oli-obk

RalfJung commented yesterday

@bors r-
FCP is still ongoing...

RalfJung commented yesterday

we can r+ right now and revert if anything comes up within the FCP

oO really? We do such things? That seems strange... why would we not follow the usual process and merge after FCP is done?

oli-obk commented yesterday

we can r+ right now and revert if anything comes up within the FCP

oO really? We do such things? That seems strange... why would we not follow the usual process and merge after FCP is done?

because I'm excited for this. We did such a thing before... I don't remember for which FCPs tho...

jhpratt commented yesterday

With due respect, being excited really isn't a good reason. FCP will likely finish soon enough and it can be merged then. It's not like this PR is critical to something.

oli-obk commented yesterday

With due respect, being excited really isn't a good reason

:D you are right. Thanks for keeping me in check

@bors r-