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

Add additional automatic conversions to usize/isize. #29220

Closed

Conversation

briansmith
Copy link
Contributor

Also, improve the documentation related to the range of usize and isize.

See rust-lang/rfcs#1218 (comment) and #28921 (comment).

I decided to be conservative and assume that 16-bit platform support might be added in the future, because C99 supports platforms with 16-bit size_t. In particular, C99 specifies that the range of size_t is at least [0, 65535]; i.e. SIZE_MAX must be at least 65,535 and so sizeof(size_t) >= 2.

Also, improve the documentation related to the range of usize and
isize.
@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 the contribution instructions for more information.

@briansmith
Copy link
Contributor Author

See https://internals.rust-lang.org/t/adding-16-bit-pointer-support/2484 where somebody is trying to add support for a 16-bit platform that (IMO) Rust should support. This is why I didn't add conversions from u32 and i32.

@alexcrichton
Copy link
Member

When discussing #28921 the libs team decided to punt on isize/usize for now until a compelling use case came up, so I'm curious but do you have a use case in mind for these conversions? (the u8/i8 ones being clearly ok of course).

Also guaranteeing that isize or usize is at least 16 bits is a language change that would need to go through an RFC most likely rather than just a PR here.

@petrochenkov
Copy link
Contributor

@alexcrichton
There's a large number of examples in petrochenkov/rust@5b56d73...eb1b670 (into is called widen or widen_ there).
Conversions u32 -> usize are the most useful, but they are not included in this PR and should be guarded by cfg if we are going to encourage the AVR port.

@briansmith
Copy link
Contributor Author

When discussing #28921 the libs team decided to punt on isize/usize for now until a compelling use case came up, so I'm curious but do you have a use case in mind for these conversions?

I mostly deal with networking protocols, and networking protocols frequently specify that certain quantities are 8 or 16 bits wide. For example., the TLS 1.2 spec has several quantities defined as uint16. Several related protocols do likewise. I generally store such quantities as u16 in my code in order to document their range. (I would actually prefer a uint_fast16_t sometimes.) Like @petrochenkov, I am working on improving the type-safety of my code by eliminating uses of the Rust as operator, because as is too error-prone for safety-critical code. This is part of that effort.

Also guaranteeing that isize or usize is at least 16 bits is a language change that would need to go through an RFC most likely rather than just a PR here.

I will post an RFC.

Conversions u32 -> usize are the most useful, but they are not included in this PR and should be guarded by cfg if we are going to encourage the AVR port.

Like I mentioned, I purposely didn't include those because they are not AVR-compatible. Regardless, you are right that there is a need for a widen like you defined that works for u32 and i32, for code that will never execute on a 16-bit platform. That includes lots of operating-system-specific code for IOS, Windows, and other platforms that don't support 16-bit systems. But, I think it would be best to do that separately, because I think it is not clear how to best support that. In particular, I definitely think it would be neat to design things in a way that encourages 16-bit-microcontroller-friendly code, but realistically a lot of developers just don't have the time or interest to deal with that.

@jonas-schievink
Copy link
Contributor

I would like an impl<T, I: Into<usize>> Index<I> for [T] in addition to this (but that might be backwards incompatible unless we also allow i32 -> usize conversions, which isn't a very sane conversion to do)

@petrochenkov
Copy link
Contributor

@briansmith

In particular, I definitely think it would be neat to design things in a way that encourages 16-bit-microcontroller-friendly code, but realistically a lot of developers just don't have the time or interest to deal with that.

Portability to 16-bit is a pretty niche concern, unlike portability between 32-bit and 64-bit. In my proposed lint-based solution (#28921 (comment), #28921 (comment)) we can have a lint for 16-bit portability too, but disabled by default.

@briansmith
Copy link
Contributor Author

Even in -mint8 mode, where int is 8 bits, size_t and ptrdiff_t are still 16-bits wide in gcc-avr; see https://gcc.gnu.org/wiki/avr-gcc. So, I think it makes sense to go ahead with this. I guess the next step is drafting the RFC.

@@ -413,6 +415,8 @@ mod prim_isize { }
//
/// The pointer-sized unsigned integer type.
///
/// `usize` is guaranteed to be at least as large as `u16`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this change to the language (and therefore RFC) is required. If we ever support a platform with 8-bit pointers (which is unlikely, but still) it would be much easier to just insert a cfg before impl_from! { u16, usize }.

For reference:

For the smallest AVR devices with less than 256 bytes of SRAM the tiny memory model
can be used in many cases. When using tiny memory model, all variables in SRAM are
accessed with 8-bit pointers instead of 16-bit pointers. This reduces the code size for
loading pointer values.

http://www.atmel.com/images/doc1497.pdf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't think this change to the language (and therefore RFC) is required. If we ever support a platform with 8-bit pointers (which is unlikely, but still) it would be much easier to just insert a cfg before impl_from! { u16, usize }.

Even if we wanted to support the tiny memory model, usize would still have to be at least 16 bits, because size_t would still be 16 bits, and usize being smaller than size_t would mean that usize is smaller than size_t, which would make dealing with the FFI in a generic way very complicated.

@alexcrichton
Copy link
Member

@briansmith, @petrochenkov

I also personally want u32/i32 -> usize/isize conversions, but that still doesn't mean it can be changed here without an RFC.

Thanks for the use cases though, they sound like good candidates for motivation in the RFC!

@eddyb
Copy link
Member

eddyb commented Oct 27, 2015

The "automatic" in the title is confusing, since this isn't some sort of a coercion, just a set of library trait impls.

@bors
Copy link
Contributor

bors commented Oct 29, 2015

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

@steveklabnik
Copy link
Member

What's the status of this PR?

@alexcrichton
Copy link
Member

Closing due to inactivity, but I think that we'll likely want to decide about usize/isize separately before taking further action on this PR.

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.

9 participants