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

Change Shl<T, T> (resp Shr<T, T>) for Int to Shl<uint, T> (resp Shr<uint, T>) #15407

Merged
merged 1 commit into from
Jul 24, 2014
Merged

Conversation

sneves
Copy link
Contributor

@sneves sneves commented Jul 4, 2014

At the moment, writing generic functions for integer types that involve shifting is rather verbose. For example, a function at shifts an integer left by 1 currently requires

use std::num::One;
fn f<T: Int>(x : T) -> T {
    x << One::one()
}

If the shift amount is not 1, it's even worse:

use std::num::FromPrimitive;
fn f<T: Int + FromPrimitive>(x: T) -> T {
    x << FromPrimitive::from_int(2).unwrap()
}

This patch allows the much simpler implementation

fn f<T: Int>(x: T) -> T { 
    x << 2
}

It accomplishes this by changing the built-in integer types (and the Int trait) to implement Shl<uint, T> instead of Shl<T, T> as it currently is defined. Note that the internal implementations of shl already cast the right-hand side to uint. BigInt also implements Shl<uint, BigInt>, so this increases consistency.

All of the above applies similarly to right shifts, i.e., Shr<uint, T>.

@erickt
Copy link
Contributor

erickt commented Jul 16, 2014

This seems pretty reasonable, but it's probably out of my wheelhouse to r+ it. Would this cause any problems if we were running Rust on an 8 bit, or 16 bit machine, where uint would map to a u8 or u16?

@sneves
Copy link
Contributor Author

sneves commented Jul 16, 2014

I don't think so. The reason is that shifts outside the range [0, bit_size - 1] are already undefined behavior (per #10183), and for builtin types every valid shift amount (all <= 63) fits within 8 bits comfortably. I don't think there would be problems even if the shifting behavior was defined to be anything reasonable (i.e., either reducing modulo bitsize or zeroing the variable).

@alexcrichton
Copy link
Member

This seems like it fits what one would actually expect out of shifting, especially now that we require the shift amount to have the type uint.

cc @aturon, I'm personally in favor of this.

@aturon
Copy link
Member

aturon commented Jul 23, 2014

I think this makes perfect sense. I'm signing off on it.

bors added a commit that referenced this pull request Jul 24, 2014
At the moment, writing generic functions for integer types that involve shifting is rather verbose. For example, a function at shifts an integer left by 1 currently requires 

    use std::num::One;
    fn f<T: Int>(x : T) -> T {
        x << One::one()
    }

If the shift amount is not 1, it's even worse:

    use std::num::FromPrimitive;
    fn f<T: Int + FromPrimitive>(x: T) -> T {
        x << FromPrimitive::from_int(2).unwrap()
    }

This patch allows the much simpler implementation

    fn f<T: Int>(x: T) -> T { 
        x << 2
    }

It accomplishes this by changing the built-in integer types (and the `Int` trait) to implement `Shl<uint, T>` instead of `Shl<T, T>` as it currently is defined. Note that the internal implementations of `shl` already cast the right-hand side to `uint`. `BigInt` also implements `Shl<uint, BigInt>`, so this increases consistency.

All of the above applies similarly to right shifts, i.e., `Shr<uint, T>`.
@bors bors closed this Jul 24, 2014
@bors bors merged commit 7b7d23c into rust-lang:master Jul 24, 2014
@pythonesque
Copy link
Contributor

Did this ever get back-applied to SIMD? It seems like you might potentially want both types of operations for SIMD.

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.

6 participants