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

Clean up special function const checks #90273

Merged
merged 2 commits into from
Oct 27, 2021
Merged

Clean up special function const checks #90273

merged 2 commits into from
Oct 27, 2021

Conversation

nbdd0121
Copy link
Contributor

@nbdd0121 nbdd0121 commented Oct 25, 2021

Mark them as const and #[rustc_do_not_const_check] instead of hard-coding them in const-eval checks.

r? @oli-obk
@rustbot label A-const-eval T-compiler

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 25, 2021
@Mark-Simulacrum
Copy link
Member

I'm not very familiar with this code, but it looks like rustc_do_not_const_check also disables post-drop elaboration checking, but this doesn't add extra checks to that code in an obvious way, so maybe this has an unintended side effect of disabling that checking?

@rust-log-analyzer

This comment has been minimized.

@nbdd0121
Copy link
Contributor Author

I'm not very familiar with this code, but it looks like rustc_do_not_const_check also disables post-drop elaboration checking, but this doesn't add extra checks to that code in an obvious way, so maybe this has an unintended side effect of disabling that checking?

#[rustc_do_not_const_check] functions shouldn't be const checked at all regardless if const_precise_live_drops is enabled.

Mark them as const and `#[rustc_do_not_const_check]` instead of hard-coding
them in const-eval checks.
@rustbot rustbot added A-const-eval Area: Constant evaluation (MIR interpretation) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 26, 2021
Comment on lines 281 to 283
// We certainly do *not* want to actually call the fn
// though, so be sure we return here.
throw_unsup_format!("calling non-const function `{}`", instance)
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the is_const_fn local and put this inside the block that sets it to true, since all the hooked fns are now const, we don't need to pass this as a parameter.

@fee1-dead
Copy link
Member

I'm not very familiar with this code, but it looks like rustc_do_not_const_check also disables post-drop elaboration checking, but this doesn't add extra checks to that code in an obvious way, so maybe this has an unintended side effect of disabling that checking?

#[rustc_do_not_const_check] functions have bodies that are not run at compile time (they are "fake" const fns), since CTFE always intercepts those functions to call another actual const fn that can be run at compile time.

I added this because I needed to use ~const bounds in the const_eval_select intrinsic, which means it must be a const fn without a const body.

@fee1-dead
Copy link
Member

r? @fee1-dead

@rust-highfive rust-highfive assigned fee1-dead and unassigned oli-obk Oct 27, 2021
Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@fee1-dead
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 27, 2021

📌 Commit 223f580 has been approved by fee1-dead

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 27, 2021
@bors
Copy link
Contributor

bors commented Oct 27, 2021

⌛ Testing commit 223f580 with merge dd757b9...

@bors
Copy link
Contributor

bors commented Oct 27, 2021

☀️ Test successful - checks-actions
Approved by: fee1-dead
Pushing dd757b9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 27, 2021
@bors bors merged commit dd757b9 into rust-lang:master Oct 27, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 27, 2021
@nbdd0121 nbdd0121 deleted the const branch October 27, 2021 18:49
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (dd757b9): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants