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

Also generate StorageDead in constants #78679

Merged
merged 1 commit into from
Dec 9, 2020
Merged

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Nov 2, 2020

r? @eddyb

None of this special casing is actually necessary since we started promoting within constants and statics.

We may want to keep some of it around out of perf reasons, but it's not required for user visible behaviour

somewhat related: #68622

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 2, 2020
@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 2, 2020

cc @rust-lang/wg-const-eval

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 2, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Nov 2, 2020

⌛ Trying commit 3f2f0dc383a6facaa1d64a21a8923d39943cc2ef with merge 0ccb4b9b8cc4437f530995e70c33db2fa684fc81...

@bors
Copy link
Contributor

bors commented Nov 2, 2020

☀️ Try build successful - checks-actions
Build commit: 0ccb4b9b8cc4437f530995e70c33db2fa684fc81 (0ccb4b9b8cc4437f530995e70c33db2fa684fc81)

@rust-timer
Copy link
Collaborator

Queued 0ccb4b9b8cc4437f530995e70c33db2fa684fc81 with parent 338f939, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (0ccb4b9b8cc4437f530995e70c33db2fa684fc81): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

@RalfJung
Copy link
Member

RalfJung commented Nov 7, 2020

Looks like we have a 5% regression in a stress test and a 1.5% regression on "ucd".

Comment on lines 439 to 452
// In constants, temp_lifetime is None. We should not need to drop
// anything because no values with a destructor can be created in
// a constant at this time, even if the type may need dropping.
// Certain dead code around "values" of `!` type will not actually have a lifetime
// associated with them.
// FIXME: can we generate one?
if let Some(temp_lifetime) = temp_lifetime {
this.schedule_drop_storage_and_value(upvar_span, temp_lifetime, temp);
}
Copy link
Member

Choose a reason for hiding this comment

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

This comment was copied from here (as part of #52405's c3dbdfe):

// In constants, temp_lifetime is None. We should not need to drop
// anything because no values with a destructor can be created in
// a constant at this time, even if the type may need dropping.
if let Some(temp_lifetime) = temp_lifetime {
this.schedule_drop_storage_and_value(
expr_span, temp_lifetime, &Place::Local(temp), expr_ty,
);
}

This is the current version, updated in (your) #56127:

// In constants, `temp_lifetime` is `None` for temporaries that
// live for the `'static` lifetime. Thus we do not drop these
// temporaries and simply leak them.
// This is equivalent to what `let x = &foo();` does in
// functions. The temporary is lifted to their surrounding
// scope. In a function that means the temporary lives until
// just before the function returns. In constants that means it
// outlives the constant's initialization value computation.
// Anything outliving a constant must have the `'static`
// lifetime and live forever.
// Anything with a shorter lifetime (e.g the `&foo()` in
// `bar(&foo())` or anything within a block will keep the
// regular drops just like runtime code.
if let Some(temp_lifetime) = temp_lifetime {
this.schedule_drop(expr_span, temp_lifetime, temp, DropKind::Storage);
}

If we don't want to copy the comment (which is pretty long), we could at least point to the other one?
Or if that's not applicable, it could still mention as_temp but say that the same situation does not arise for what's handled here.

@eddyb
Copy link
Member

eddyb commented Nov 17, 2020

I think this is strictly "more correct", even if less efficient. r=me if you want to land it as-is (modulo that one comment) but it might be prudent to wait for more opinions from @rust-lang/compiler.

@nikomatsakis
Copy link
Contributor

I'm in favor of removing the special casing

@oli-obk oli-obk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 30, 2020
@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 9, 2020

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Dec 9, 2020

📌 Commit 84fe7cf has been approved by eddyb

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 9, 2020
@bors
Copy link
Contributor

bors commented Dec 9, 2020

⌛ Testing commit 84fe7cf with merge cc03ee6...

@bors
Copy link
Contributor

bors commented Dec 9, 2020

☀️ Test successful - checks-actions
Approved by: eddyb
Pushing cc03ee6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 9, 2020
@bors bors merged commit cc03ee6 into rust-lang:master Dec 9, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants