-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Allow comparisons between integers of different types #2021
Closed
Closed
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
f5e9a15
template copied
VFLashM 31d92e4
all but motivation added
VFLashM 605f3e8
better motivation
VFLashM d4fd71c
grammar fix
VFLashM cccd48e
rfc file moved into right dir
VFLashM 54bef7d
signed->unsigned typo fixed, Ord/Eq mentioned as optional, unresolved…
VFLashM ca6b574
widening to signed added as a more efficient way to compare signed/un…
VFLashM c6af879
added note about branching into implementation, explicit branching re…
VFLashM d2612f5
how we teach section updated, short description of potential changes
VFLashM File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
- Feature Name: heterogeneous_comparisons | ||
- Start Date: 2017-06-06 | ||
- RFC PR: (leave this empty) | ||
- Rust Issue: (leave this empty) | ||
|
||
# Summary | ||
[summary]: #summary | ||
|
||
Allow to compare integer values of different signedness and size. | ||
|
||
# Motivation | ||
[motivation]: #motivation | ||
|
||
Right now every comparison between different integral types requires a cast. These casts don't only clutter the code, but also encourage writing incorrect code. | ||
|
||
The easiest way to compare signed and unsigned values is to cast unsigned value to signed type. It works most of the time, but it will silently ignore overflows. | ||
|
||
Comparison between values of different integer types is always well-defined. There is only one correct result and only one way to get it. Allowing compiler to perform these comparisons will reduce both code clutter and count of hidden overflow/underflow errors users make. | ||
|
||
# Detailed design | ||
[design]: #detailed-design | ||
|
||
`PartialEq` and `PartialOrd` should be implemented for all pairs of signed/unsigend 8/16/32/64/(128) bit integers and `isize`/`usize` variants. | ||
|
||
Implementation for signed-singed and unsigned-unsigned pairs should promote values to larger type first, then perform comparison. | ||
|
||
Example: | ||
|
||
``` | ||
fn less_than(a: i8, b: i16) -> bool { | ||
(a as i16) < b | ||
} | ||
``` | ||
|
||
Implementation for signed-unsigned pairs where unsigned type is smaller than machine word size can promote both values to smallest signed type fully covering values of both argument types, then perform comparison. | ||
|
||
Example: | ||
|
||
``` | ||
fn less_than(a: i8, b: u16) -> bool { | ||
// i32 is the smallest signed type | ||
// which can contain any u16 value | ||
(a as i32) < (b as i32) | ||
} | ||
``` | ||
|
||
Implementation for signed-unsigned pairs where unsigned type is as big as machine word size or larger should first check if signed value less than zero. If not, then it should promote both values to unsigned type with the same size as larger argument type and perform comparison. For most platforms it should be possible to implement it without actual branching. | ||
|
||
Example (for 32-bit system): | ||
|
||
``` | ||
fn less_than(a: i64, b: u32) -> bool { | ||
(a < 0) || ((a as u64) < (b as u64)) | ||
} | ||
``` | ||
|
||
|
||
Optionally `Ord` and `Eq` can be modified to allow `Rhs` type not equal to `Self`: | ||
|
||
``` | ||
pub trait Eq<Rhs = Self>: PartialEq<Rhs> { } | ||
|
||
pub trait Ord<Rhs = Self>: Eq<Rhs> + PartialOrd<Rhs> { | ||
fn cmp(&self, other: &Rhs) -> Ordering; | ||
} | ||
``` | ||
|
||
# How We Teach This | ||
[how-we-teach-this]: #how-we-teach-this | ||
|
||
As of now I can't find anything about comparisons in "The Rust Programming Language". The most reasonable place seems to be in chapter "3.2 Data Types", in or just after "Numeric Operations" paragraph (for second edition). | ||
|
||
We just need to mention that Rust allows comparison between different numeric types and that it returns mathematically correct result. Optionally, potential overhead can be mentioned, even though I'm not sure that's the right place to mention it. | ||
|
||
Rust reference seems to describe language itself and doesn't go into details about any traits/operators implementation. I can't find any reasonable place to cover it there. | ||
|
||
# Drawbacks | ||
[drawbacks]: #drawbacks | ||
|
||
* It might break some code relying on return type polymorphism. It won't be possible to infer type of the second argument from type of the first one for `Eq` and `Ord`. | ||
* Correct signed-unsigned comparison requires one more operation than regular comparison. Proposed change hides this performance cost. If user doesn't care about correctness in his particular use case, then cast and comparison is faster. | ||
* The rest of rust math prohibits mixing different types and requires explicit casts. Allowing heterogeneous comparisons (and only comparisons) makes rust math somewhat inconsistent. | ||
* For some platforms it might be necessary or more efficient to use branching in signed-unsigned comparisons (arm thumb?). Using branching will turn comparison into a non-constant-time operation. Any value-dependant operation in cryptography code is a potential security risk because of timing attacks. On other hand, on some platforms even multiplication is not guaranteed to be a constant-time operation. | ||
|
||
# Alternatives | ||
[alternatives]: #alternatives | ||
|
||
* Keep things as is. | ||
* Add generic helper function for cross-type comparisons. | ||
|
||
# Unresolved questions | ||
[unresolved]: #unresolved-questions | ||
|
||
* Is `PartialOrd` between float and int values as bad idea as it seems? | ||
* Which platforms might need branching in signed-unsigned comparison implementation? |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: "signed-singed" should be "signed-signed"