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.

Sign up for GitHub

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
wants to merge 9 commits into from
Closed

Allow comparisons between integers of different types #2021

Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f5e9a15
template copied
VFLashM Jun 7, 2017
31d92e4
all but motivation added
VFLashM Jun 7, 2017
605f3e8
better motivation
VFLashM Jun 7, 2017
d4fd71c
grammar fix
VFLashM Jun 7, 2017
cccd48e
rfc file moved into right dir
VFLashM Jun 7, 2017
54bef7d
signed->unsigned typo fixed, Ord/Eq mentioned as optional, unresolved…
VFLashM Jun 7, 2017
ca6b574
widening to signed added as a more efficient way to compare signed/un…
VFLashM Jun 7, 2017
c6af879
added note about branching into implementation, explicit branching re…
VFLashM Jun 8, 2017
d2612f5
how we teach section updated, short description of potential changes
VFLashM Jul 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions text/0000-heterogeneous-comparisons.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
- 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

`Ord` and `Eq` traits should be modified to allow `Rhs` type not equal to `Self`.

After that `PartialEq`, `Eq`, `PartialOrd` and `Ord` 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.

Implementation for signed-unsigned pairs should first check if signed value less than zero. If not, then it should promote both values to signed type with the same size as larger argument type and perform comparison.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that promotion should be to the "unsigned" type since an unsigned type can hold values larger than a signed type of the same size.

Copy link

Choose a reason for hiding this comment

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

+1

Copy link
Author

Choose a reason for hiding this comment

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

Both signed and unsigned types can hold values other type cannot.
You either have to check that signed value is non-negative then cast to unsigned, or have to check that unsigned value is not greater than maximum value of signed type.

Example:

fn less1(a: i32, b: u32) -> bool {
    if a < 0 {
        return true;
    } else {
        return (a as u32) < b;
    }
}

fn less2(a: i32, b: u32) -> bool {
    if b > std::u32::MAX {
        return true;
    } else {
        return a < (b as i32);
    }
}

I personally find check for non-negativeness a bit cleaner.

Copy link
Member

Choose a reason for hiding this comment

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

You mean std::i32::MAX as u32?

Copy link

@ssokolow ssokolow Jun 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I personally find check for non-negativeness a bit cleaner.

It's also less prone to the typo of checking whether a u32 is greater than std::u32::MAX rather than std::i32::MAX.

EDIT: ...and I just realized that's what @eddyb meant by his comment.

Copy link

@le-jzr le-jzr Jun 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@VFLashM You have a typo in the RFC. It says "promote both values to signed type", which actually overflows for b > std::i32::MAX.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, darn. Your're right. That's a typo in RFC. I absolutely meant promotion to unsigned.
Thanks.

Copy link
Contributor

@glaebhoerl glaebhoerl Jun 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@VFLashM

The RFC says:

If not, then it should promote both values to signed type

Your first code snippet says:

return (a as u32) < b;

I think this is all that @TimNN was saying: after checking for less-than-zero, the as afterwards should be to the unsigned type, not the signed type as the RFC currently says.

EDIT: the previous two comments hadn't appeared when I wrote this :)


# How We Teach This
[how-we-teach-this]: #how-we-teach-this

No changes I can find or think of.

# 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.

# Alternatives
[alternatives]: #alternatives

* Keep things as is.
* Add generic helper function for cross-type comparisons.

# Unresolved questions
[unresolved]: #unresolved-questions

Is `Ord` between float and int values as bad idea as it seems?