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.

Sign up for GitHub

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

Jump to bottom

Revert rust-lang/rust#46833 on stable #48378

Closed
wants to merge 1 commit into from
Closed

Revert rust-lang/rust#46833 on stable #48378

wants to merge 1 commit into from

Conversation

nikomatsakis
Copy link
Contributor

This is a minimal diff to disable #46833 on stable (due to #48251). I have a more expansive diff for master that allows us to opt back in.

r? @Mark-Simulacrum

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • Pull requests are usually filed against the master branch for this repo, but this one is against stable. 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 Feb 20, 2018
This was referenced Feb 20, 2018
Closed
Merged
Mark-Simulacrum
Mark-Simulacrum previously requested changes Feb 20, 2018
View reviewed changes
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

Could you make the same change for the beta PR?

src/test/run-pass/abort-on-c-abi.rs Outdated
@@ -12,6 +12,7 @@
// we never unwind through them.

// ignore-emscripten no processes
// ignore FIXME rust-lang/rust#48251 -- temporarily disabled
Copy link
Member

Choose a reason for hiding this comment

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

This syntax appears to not be supported; we have to use // ignore-test instead I think...

@nikomatsakis
Copy link
Contributor Author

@Mark-Simulacrum done

carols10cents
carols10cents reviewed Feb 21, 2018
View reviewed changes
src/librustc_mir/build/mod.rs Outdated
@@ -372,7 +372,9 @@ fn should_abort_on_panic<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
// unwind anyway. Don't stop them.
if tcx.has_attr(tcx.hir.local_def_id(fn_id), "unwind") { return false; }

return true;
// FIXME(rust-lang/rust#48251) -- Had to disable abort-on-panic
// for backwardsa compatibility reasons.
Copy link
Member

Choose a reason for hiding this comment

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

typo, not a big deal :)

backwardsa

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you KIDDING? That is the biggest of deals.

@nikomatsakis nikomatsakis added the T-core Relevant to the core team, which will review and decide on the PR/issue. label Feb 21, 2018
@nikomatsakis
Copy link
Contributor Author

@rfcbot fcp merge

I propose that we merge this and issue a new point release -- this decision also applies to the beta PR and master PR.

@rfcbot
Copy link

rfcbot commented Feb 21, 2018 •
edited by nrc
Loading

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

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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 rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Feb 21, 2018
@nikomatsakis nikomatsakis mentioned this pull request Feb 21, 2018
Merged
@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 22, 2018
@rfcbot
Copy link

rfcbot commented Feb 22, 2018

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

@ashleygwilliams
Copy link
Member

@rfcbot reviewed

@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Feb 22, 2018
Merged
@Mark-Simulacrum
Copy link
Member

Closing in favor of #48445.

bors added a commit that referenced this pull request Feb 26, 2018
@bors
Auto merge of #48379 - nikomatsakis:issue-48251-beta, r=Mark-Simulacrum
62ad16c 
[beta] temporarily disable #46833 due to #48251

see also #48378

r? @Mark-Simulacrum
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carols10cents carols10cents carols10cents left review comments

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-core Relevant to the core team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nikomatsakis @rust-highfive @rfcbot @ashleygwilliams @Mark-Simulacrum @carols10cents