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

In panicking.rs, change to usize from uint #23170

Closed
wants to merge 2 commits into from
Closed

In panicking.rs, change to usize from uint #23170

wants to merge 2 commits into from

Conversation

strega-nil
Copy link
Contributor

No description provided.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

@rprichard
Copy link
Contributor

We really should change that parameter to u32.

Edit: of course, usize is still an improvement.

@strega-nil
Copy link
Contributor Author

@rprichard Yeah, you're right. Let me fix that.

@rprichard
Copy link
Contributor

... but to change it to u32, you need to update all the other places that are using usize. Grep for panic_fmt -- for example, the libstd/rt/unwind.rs code uses usize for the line number. I'm pretty sure panic_fmt also shows up in the docs. The compiler knows about the panic_fmt lang item, but I don't know whether it expects a particular type signature; I'm guessing not?

The line as u32 cast isn't necessary.

I think it'll be necessary to use the #[cfg(stage0)] / #[cfg(not(stage0))] tags, so we can keep using usize for the stage0 libraries while using u32 for the stage1 and stage2 libraries. For example, see this commit: 647e54d

@rprichard
Copy link
Contributor

The panic! macro in libstd/macros.rs needs updating.

I think it'll be necessary to use the #[cfg(stage0)] / #[cfg(not(stage0))] tags, so we can keep using usize for the stage0 libraries while using u32 for the stage1 and stage2 libraries.

Or maybe not. If the stage0 compiler doesn't care about the type signature of panic_fmt, then maybe the usize->u32 change is completely contained inside the libraries.

@strega-nil
Copy link
Contributor Author

Fine, I need to actually do work on this. Just let the PR be for now, or
close it, and I'll fix it later.
On Mar 8, 2015 10:28 AM, "Ryan Prichard" notifications@github.com wrote:

The panic! macro in libstd/macros.rs needs updating.

I think it'll be necessary to use the #[cfg(stage0)] / #[cfg(not(stage0))]
tags, so we can keep using usize for the stage0 libraries while using u32
for the stage1 and stage2 libraries.

Or maybe not. If the stage0 compiler doesn't care about the type signature
of panic_fmt, then maybe the usize->u32 change is completely contained
inside the libraries.


Reply to this email directly or view it on GitHub
#23170 (comment).

@bors
Copy link
Contributor

bors commented Mar 28, 2015

☔ The latest upstream changes (presumably #23796) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

This appears to have migrated in the meantime, so closing. Thanks though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants