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

Remove sys::begin_unwind_? #4427

Closed
catamorphism opened this issue Jan 10, 2013 · 5 comments · Fixed by #9803
Closed

Remove sys::begin_unwind_? #4427

catamorphism opened this issue Jan 10, 2013 · 5 comments · Fixed by #9803
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.

Comments

@catamorphism
Copy link
Contributor

I presume what was intended here is to merge it into begin_unwind; comment says "temporary until rt_fail_ goes away". However, rt_fail_ hasn't gone away yet.

@catamorphism
Copy link
Contributor Author

So, rt_fail_ appears to have gone away, but I can't figure out what was intended by the original fixme. begin_unwind_ is still used and takes a string, filename, and line number, whereas the begin_unwind method on Unwinder doesn't. @graydon ?

@pnkfelix
Copy link
Member

@catamorphism From reading commit 3fcdb7d where the original FIXME was injected and which introduced begin_unwind_, it looks like begin_unwind_ was originally called from only two places: rt_fail_ and begin_unwind.

So I suspect the intention of the FIXME was to say "once we get rid of rt_fail_, we should manually inline the body of begin_unwind_ into begin_unwind and then get rid of begin_unwind_ itself.

I'll try to do that now.

@pnkfelix
Copy link
Member

Of course, commit 3fcdb7d also ensured that its begin_unwind and begin_unwind_, which both lived in sys.rs, had the same signature. I poked around a little, it looks to me like @catamorphism was talking about a different begin_unwind method of the Unwinder struct (currently in std::rt::task though at the time of @catamorphism's comment it was in core::rt::local_services).

Anyway, at this point, there are now six callers of begin_unwind_; two of them live in sys.rs right next to begin_unwind_ (those probably replaced the earlier begin_unwind from commit 3fcdb7d), a third is fail_ from libstd/unstable/lang.rs, and the remaining three are in std::rt::borrowck (my reaction to that last one: "reallly? weird.")

@pnkfelix
Copy link
Member

In case it is not clear, I was visiting this bug for triage last week, for 2013-07-15. I had thought it would be easy to fix this based on my analysis from my earlier comment. But further investigation revealed that new external callers of begin_unwind_ have been introduced. I spent some time trying to at least change the interface used by the external callers to not use unsafe code (e.g. not use native pointers to null terminated strings) but that did not appear to be an easy local transformation in the majority of the cases.

@brson
Copy link
Contributor

brson commented Oct 10, 2013

This was never nominated and isn't really a backwards compatibility issue as long is this is an unstable API. Removing milestone.

bors added a commit that referenced this issue Oct 11, 2013
This change was waiting for privacy to get sorted out, which should be true now
that #8215 has landed.

Closes #4427
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants