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

Remove uses of uint that are not pointer-related from the Int trait #20211

Closed
wants to merge 1 commit into from

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Dec 24, 2014

Use u32 instead.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@huonw
Copy link
Member

huonw commented Dec 24, 2014

Strictly speaking these are actually pointer/memory related, since one could have up to (very approximately) uint::MAX bits in an integer. That consideration may be too pedantic though.

@thestinger
Copy link
Contributor

@huonw: If the traits hadn't dropped support for non-primitive types it might make sense to keep it this way. It's fine either way now, since both types are guaranteed to be large enough.

@tbu-
Copy link
Contributor Author

tbu- commented Dec 24, 2014

@huonw That way, we need more than uint to represent the maximum number of bits.

@thestinger
Copy link
Contributor

@tbu-: That's a good point, the number of possible bits in a single object is directly tied to the number of bytes but uint can't index them all.

@thestinger
Copy link
Contributor

You need an integer type with at least pointer_size + 2 bits (since Rust limits object size to int::MAX).

@huonw
Copy link
Member

huonw commented Dec 24, 2014

I broadly agree, which is why I said 'strictly'. :)

You need an integer type with at least pointer_size + 7 bits (since Rust limits object size to int::MAX).

Only pointer_size + 2, I believe.

@thestinger
Copy link
Contributor

The length of bit sets would a good location to level the double-width integer support in LLVM and compiler-rt. It supports 64-bit integers in software when there are 32-bit hardware integers (which Rust exposes via u64/i64) and 128-bit integers in software when there are 64-bit hardware integers (which is a missing feature).

@thestinger
Copy link
Contributor

@huonw: Ah, right. I got missed up and added 8 instead of the power of 2.

@retep998
Copy link
Member

Maybe we should use an associated type so that primitive types don't need double wide integers to represent their bits while people using bignum can have their double wide integers?

@thestinger
Copy link
Contributor

@retep998: Well, these traits used to have support for big integers and other arbitrary precision types but it was all changed to taking self and other parameters by-value so it's limited to the primitive types now. If that's not going to be changed then there's no need to make it generic. These traits aren't actually usable for writing generic numeric code - they're just where the methods happen to be defined.

@tomjakubowski
Copy link
Contributor

@thestinger I see you making the claim that the traits exclude big numbers because the methods take self by value, but I don't understand why the traits can't simply be implemented for &BigNumber or whatever. I must not be seeing the whole picture.

@tbu-
Copy link
Contributor Author

tbu- commented Dec 25, 2014

@tomjakubowski It's not possible because of other methods, e.g. add(Self, Self) -> Self, you can't return a &BigNumber as a result of an addition.

@tomjakubowski
Copy link
Contributor

OK, yes, that is what I was missing. Thanks.

@thestinger
Copy link
Contributor

@tomjakubowski: It won't work because there are functions returning Self. Another issue is that you can't write generic code if the signatures don't match up in a sane way across types. For example, with the iterator types you sometimes need to convert from an iterator of &T to T but you don't want to do that if you already have T.

@thestinger
Copy link
Contributor

Treating everything as values only works for iterators because the adaptors don't generally do anything with the values beyond passing them to a closure or passing them along. It didn't work out for methods like min, max, sum and product because they weren't able to cope with this sanely. You need something like max_by / min_by or fold where it's opaque and the caller supplies the closure. Generic numeric code can't cope well with this problem at all.

@bluss
Copy link
Member

bluss commented Dec 25, 2014

The Shr and Shl operators use uint for the shift parameter. This change would make .rotate_left() etc inconsistent with the shift operators.

I also see code using bit shifts derived from a uint size_of value in the diff, and that's completely natural to me -- I honestly think this change is going the wrong way. Stick with uint.

@tbu-
Copy link
Contributor Author

tbu- commented Dec 25, 2014

@bluss Shl and Shr will need to be adjusted too, bit shifts are nothing pointer related.

@tbu-
Copy link
Contributor Author

tbu- commented Dec 25, 2014

Again, think of how it would look like if it wasn't such a nice name uint:

fn shift_left(value: u32, shift: uintptr_t) -> u32

In this case, everybody would ask why one would choose uintptr_t here, it's just the nice name uint that makes us argue to keep it. :/

@bluss
Copy link
Member

bluss commented Dec 25, 2014

size_of returns "uintptr_t" already, and the bit counts are related to the size of the type (already mentioned above).

@tbu-
Copy link
Contributor Author

tbu- commented Dec 25, 2014

@bluss size_of is doing so correctly. The number of bits in a primitive data type has nothing to do with memory size, as it's always <=64 currently. The number of bits in any other data type cannot be represented with Rust's current integer types. So we go for u32 as it fits good for the use case of number of bits in a primitive data type.

@aturon
Copy link
Member

aturon commented Jan 5, 2015

@tbu- Thanks for doing this! Assuming that rust-lang/rfcs#544 is accepted, we're going to need to (1) establish formal guidelines for integer usage and (2) do a full sweep of std to roll them out during alpha. (This is part of the more general rollout plan.) Do you mind waiting on this PR until a conventions RFC is accepted? (I expect this to happen quickly after alpha, if not before.)

@tbu-
Copy link
Contributor Author

tbu- commented Jan 5, 2015

@aturon I'm totally fine with this, and I'm happy that this is talked about seriously.

@tbu-
Copy link
Contributor Author

tbu- commented Jan 5, 2015

@aturon Should I close this PR or just wait?

@aturon
Copy link
Member

aturon commented Jan 5, 2015

@tbu- Please close for now, it helps speed bors up, and then reopen later on when the convention is clear. Thanks again!

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