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

Implement conversion traits for primitive integer types #28921

Merged
merged 1 commit into from
Oct 15, 2015

Conversation

petrochenkov
Copy link
Contributor

impl_from! { u32, usize }
#[cfg(target_pointer_width = "64")]
impl_from! { u64, usize }
#[cfg(target_pointer_width = "16")]
Copy link
Contributor

Choose a reason for hiding this comment

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

we have support for 16 bit?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, we don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tbu-
Copy link
Contributor

tbu- commented Oct 9, 2015

Very cool. This makes it possible to get rid of lots of as casts for non-truncating casts.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 9, 2015

Very cool. This makes it possible to get rid of lots of as casts for non-truncating casts.

this screams for a lint...

@petrochenkov
Copy link
Contributor Author

@oli-obk

this screams for a lint...

Yes, but not right now, because now we can write large + small as Large, but can't replace it with large + small.into() or large + small.into(): Large yet. Besides, the lint can give platform-dependent warnings (this is solved by cast() in the RFC linked above).

@japaric
Copy link
Member

japaric commented Oct 9, 2015

but can't replace it with large + small.into() or large + small.into(): Large yet.

But we can write large + Large::from(small) which is one character smaller than the type ascription version 😄.


Nice to see lossless int conversions land in std. I've been using my own From trait that does this + checked casts (return Option), but it would be nice to be able drop that dependency when I only need lossless int casts.

this screams for a lint...

I treat as the same as unsafe (i.e. must be audited) and try to minimize its use in my code; instead, I use lossless/checked usize::froms wherever possible. And, I usually do this by grepping my code for the as keyword because I'm too lazy to (learn how to) write a lint. So, I would very much welcome such a lint. (Then I can copy it and modify it for my needs 😈)

impl_from! { u32, u64 }
#[cfg(any(target_pointer_width = "64", target_pointer_width = "32"))]
impl_from! { u32, usize }
#[cfg(target_pointer_width = "64")]
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I am a bit wary of taking this step: it makes it easy to silently/accidentally introduce arch-specific code. (Contrast this with the approach we took with OS-specific extensions, where you generally have to import a specific trait to "opt in" to platform specificity.)

cc @alexcrichton

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was discussed in the RFC and tested on rustc Windows 64-bit / Linux 32-bit. Yes, porting to a new platform assumes changing some into into cast/as, but potential porting bugs are also uncovered.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I would prefer to avoid usize and isize in these "always working" coercions for now. I'm not actually aware of any types which have some traits implemented on some platforms and not others, and that's a great boon for portability so it'd be nice to keep it that way!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is significant and important part of the RFC, it's motivated and tested in practice, I won't surrender it just like that. I'd argue that most of integer conversions actually involve usize.

Any comments from @japaric? Do your own conversion traits perform conversions to/from usize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's some old statistics: https://internals.rust-lang.org/t/the-operator-as-statistics-of-usage/1353
as usize is very popular, including indexing context v[a as usize].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Too bad implemented trait methods can't be marked with a warning message attribute, a lint.
Something like "you use a method making you code non-portable between 32-bit and 64-bit".
If user isn't interested in this kind of portability (or the types are different on different platforms so the conversion is always lossless even if it would not be lossless if the types were the same, but the lint doesn't know that) he could explicitly silence the warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, how do you think, if I write a specialized built-in portability lint for this, would it be a good idea? (This lint could detect other 32<->64 portability problems eventually).

Copy link
Member

Choose a reason for hiding this comment

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

Linting sounds like a reasonable way to approach this problem, for sure; it's an idea that's been tossed around not just for this portability issue, but also to make things like implicit widening part of the language but allow people to "opt out" via a lint.

It'd be awesome if this infrastructure could be used in e.g. the stability lint, which currently is not sensitive to the stability of trait impls. I'm not sure how much work that would be involved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so I'm going to comment out the impls problematic for 32<->64 portability for now to merge this PR and will return later with a lint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aturon
Done.

@aturon
Copy link
Member

aturon commented Oct 9, 2015

cc @rust-lang/libs When we discussed this RFC briefly this week, we didn't talk in detail about how usize should be treated. I'd appreciate additional input on this aspect of the design.

@tbu-
Copy link
Contributor

tbu- commented Oct 11, 2015

With the controversial parts removed, this should be fine to land, right?

@aturon
Copy link
Member

aturon commented Oct 13, 2015

@tbu- Most likely, though I would like to get broader feedback on the 32bit -> usize conversion. I'm going to flag this for a libs team decision at the meeting tomorrow.

@aturon aturon added I-needs-decision Issue: In need of a decision. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 13, 2015
@Gankra
Copy link
Contributor

Gankra commented Oct 13, 2015

(fwiw we've long held that 16-bit isn't supported. I can't recall any specific past decisions that legitimately preclude it, but I feel like they exist)

@brson brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 14, 2015
@alexcrichton alexcrichton removed the I-needs-decision Issue: In need of a decision. label Oct 15, 2015
@alexcrichton
Copy link
Member

The libs team talked about this today and our conclusion was that at this time we'd like to avoid any #[cfg] impl blocks entirely. Along those lines we also think that implementations for isize and usize should be avoided at this time. We can always add them later, but it's less obvious that we want to add them right now.

It sounded like one of the main use cases of From and Into with usize/isize were around indexing, but @sfackler brought up a good point where we could certainly implement Index<u64> for vectors and slices today, it's just have extra checks on 32-bit platforms to test if it's out of bounds.

@petrochenkov could you perhaps elaborate a bit on the use cases you're thinking of for isize and usize? Perhaps they can fit into patterns like Index above with slices?

@petrochenkov
Copy link
Contributor Author

@alexcrichton
Updated with completely platform independent version.

could you perhaps elaborate a bit on the use cases you're thinking of for isize and usize?

We can always argue later, but it's less obvious that we want to do it right now

we could certainly implement Index<u64> for vectors and slices

Please, no

@alexcrichton
Copy link
Member

@bors: r+ 6f3e84d

Thanks!

@bors
Copy link
Contributor

bors commented Oct 15, 2015

⌛ Testing commit 6f3e84d with merge fa9a421...

@briansmith
Copy link
Contributor

@petrochenkov could you perhaps elaborate a bit on the use cases you're thinking of for isize and usize? Perhaps they can fit into patterns like Index above with slices?

I am not @petrochenkov. But, in some cases in my code I have static size values of type u8 or u16 (to keep structures small) that I later need to do usize arithmetic and/or indexing on. Therefore, it would be very useful to have u8 and u16 implicitly convertible to usize.

fwiw we've long held that 16-bit isn't supported

If so, could we please add the u16 -> usize/isize, u32 -> usize, and i32 -> isize conversions that were not added?

@briansmith
Copy link
Contributor

If so, could we please add the u16 -> usize/isize, u32 -> usize, and i32 -> isize conversions that were not added?

My PR #29220 adds conversions from u16 to usize, i16 to isize, and u8 to isize. I did not add any conversions from i32 or u32 because there actually is work underway to port Rust to AVR, which uses a 16-bit address space; see https://internals.rust-lang.org/t/adding-16-bit-pointer-support/2484.

@briansmith
Copy link
Contributor

we could certainly implement Index for vectors and slices
Please, no

I agree. In particular, that shouldn't be added if it means having a runtime check for 32-bit-and-smaller platforms.

bors added a commit that referenced this pull request Oct 29, 2015
This is a spiritual successor to #28921, completing the "upcast" idea from rust-num/num#97.
@petrochenkov petrochenkov deleted the intconv branch November 22, 2015 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.