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

variant of PR 87770 with limited scope for backport. #91136

Closed

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Nov 22, 2021

This version does not attempt to resolve the problem that is illustrated by src/test/ui/const-generics/generic_const_exprs/drop_impl.rs (a problem that relies on nightly features to demonstrate anyway).

My thinking is that this is a better choice for backporting to stable and beta.

Fix #90838 (which was closed by accident by #90840, which only landed on nightly, so far).

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

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

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • Pull requests are usually filed against the master branch for this repo, but this one is against beta. Please double check that you specified the right target!

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 22, 2021
@pnkfelix pnkfelix force-pushed the limit-scope-of-87770-backport branch from e96fa7b to f9ee681 Compare November 22, 2021 18:06
@pnkfelix
Copy link
Member Author

(The compiler team itself hasn't discussed whether we would be better off backporting this, or backporting PR #90840 itself. I believe we will resolve that question on Wednesday.)

@Mark-Simulacrum
Copy link
Member

I'm probably not a good reviewer here -- r? @jackh726 perhaps?

@jackh726
Copy link
Member

This implementation looks good to me, but I don't know if its strictly better than #90840 for backport.

Does the new test you added also pass on master? We probably want that test added regardless.

On one hand, this is somewhat of a revert to #87770, which is "safer". On the other, it's pretty clear that the cause of the issue was just a failure to relate the lifetimes.

I'll leave the decision to backport this or #90840 to the compiler team, but I personally prefer the latter out of consistency. However, r=me on this implementation.

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 24, 2021
@pnkfelix
Copy link
Member Author

This implementation looks good to me, but I don't know if its strictly better than #90840 for backport.

Does the new test you added also pass on master? We probably want that test added regardless.

The test is taken from #90840. So it already is on master.

@pnkfelix
Copy link
Member Author

(Closing; We'll backport #90840 instead of this.)

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. 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.

6 participants