Audit integer types in the standard library #22240

Closed
nikomatsakis opened this Issue Feb 12, 2015 · 22 comments

Projects

None yet

8 participants

@nikomatsakis
Contributor

Below is a list of modules that contain text matching the regular expression \bu?int\b and which are intended to be stable. In some cases, these terms only appear in comments (which should still be fixed). In some cases, I have simply put .. after a directory to indicate all files within that directory. Please sign up for modules in comments below and I will endeavor to keep this list up to date with who is working on what (if you have sufficient privileges, feel free to edit the comment directly).

Guidelines to follow

These are the guidelines that we intend to follow in the standard library. These guidelines are not intended as universal guidelines to be used outside the standard library (though of course one might choose to do so).

  1. Use unsigned values if the value should always be greater than or equal to zero, and signed values otherwise.
  2. For indices, pointers, or other values which are tied to a data structure whose size is proportional to the size of memory, use usize or isize.
  3. For cases where the acceptable domain of a value perfectly fits a fixed number of bits, use the appropriate fixed-size type. For example, a method like write_u16 would take a u16 argument.
  4. Otherwise, use i32/u32 if the value has a narrow range and i64/u64 otherwise.

Examples:

  • Vector indices and hashmap lengths use usize.
  • The size of a file would be u64, as the maximum size of a file is not tied to addressable memory.
  • Something like write_u16 takes a u16 (shocker, I know!)
  • The radix of an integer would use u32.
    • You might expect u8, since a radix higher than 256 is not useful, but the domain of useful radices is actually much smaller than u8, so using u8 isn't providing a meaningful guarantee, and will simply increase friction.

Module listing

@nikomatsakis
Contributor

Nominating for the 1.0 beta milestone.

@Gankro
Contributor
Gankro commented Feb 12, 2015

I'll finish up collections. Looks like I just missed some docs stuff.

@aturon aturon referenced this issue Feb 12, 2015
Closed

Stabilization for 1.0-alpha2 #20761

29 of 38 tasks complete
@pnkfelix
Member

1.0 beta, P-backcompat-lib.

@pnkfelix pnkfelix added this to the 1.0 beta milestone Feb 12, 2015
@pnkfelix pnkfelix removed the I-nominated label Feb 12, 2015
@aturon aturon self-assigned this Feb 12, 2015
@nikomatsakis
Contributor

cc @nick29581 @pnkfelix @aturon @alexcrichton @pcwalton @steveklabnik @wycats don't forget to sign up for somethin' ;)

@petrochenkov
Contributor

I'll take libunicode, char, str and ascii
EDIT: they are coupled with fmt and num, so I'm fixing some bits there too

@pnkfelix
Member

@nikomatsakis some of the uses of int/uint are just arising from what seem to be dummy types in examples in the documentation. For example this in cell.rs where the edges in a hypothetical graph structure are (uint, uint) (and edge.0/edge.1 are not being used for array indexing).

I sympathize with not wanting to jump into fully generic code in these examples.

But for consistency in the docs overall, what do we want our go-to type to be in these cases: u32, i32, or ... ? (I'm guessing i32 for consistency with integer fallback.

@Gankro
Contributor
Gankro commented Feb 14, 2015

Yes, my general rule when updating the docs is to try to have integer-fallback trigger and have i32 inferred, with explicit i32 when necessary.

So e.g. I use a lot of let vec: Vec<_> = [1, 2, 3, 4].iter().cloned().collect();

@Manishearth Manishearth added a commit to Manishearth/rust that referenced this issue Feb 15, 2015
@Manishearth Manishearth Rollup merge of #22350 - brson:usize, r=Gankro 8173aed
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this issue Feb 15, 2015
@Manishearth Manishearth Rollup merge of #22339 - petrochenkov:int, r=huonw
 Some function signatures have changed, so this is a [breaking-change].
In particular, radixes and numerical values of digits are represented by `u32` now.

Part of #22240
b862189
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this issue Feb 15, 2015
@Manishearth Manishearth Rollup merge of #22350 - brson:usize, r=Gankro c59f62b
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this issue Feb 15, 2015
@Manishearth Manishearth Rollup merge of #22339 - petrochenkov:int, r=huonw
 Some function signatures have changed, so this is a [breaking-change].
In particular, radixes and numerical values of digits are represented by `u32` now.

Part of #22240
f7870b6
@nikomatsakis
Contributor

@pnkfelix as @Gankro said, I've mostly used u32/i32 as my go to type for docs, if it seems that "generic integer" is all that's requested.

EDIT: Or, I thought I did. I guess I left some as usize in #22294 ...

@vojtechkral
Contributor

libcore/macros.rs → cc #19284 , which is tightly related to how types are used in codemap.rs: the Pos trait uses usize, BytePos uses u32 and CharPos uses usize. Why are these types chosen the way they are? Could they perhaps be changed so that #19284 is easier to fix?

EDIT: Anyway, once #19284 is fixed, libcore/macros.rs will have been taken care of, there are no other uints/ints in that file.

@Manishearth Manishearth added a commit to Manishearth/rust that referenced this issue Feb 17, 2015
@Manishearth Manishearth Rollup merge of #22401 - pnkfelix:fsk-int-uint-audit, r=Gankro 812fa82
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this issue Feb 17, 2015
@Manishearth Manishearth Rollup merge of #22401 - pnkfelix:fsk-int-uint-audit, r=Gankro c0865df
@GuillaumeGomez
Member

I'll take libstd/lib.rs.

@aturon aturon referenced this issue Feb 18, 2015
Closed

Stabilization for 1.0-beta #22500

75 of 91 tasks complete
@GuillaumeGomez
Member

I take libcore/lib.rs too, libcore/panicking.rs and libstd/thread.rs.

@brson
Contributor
brson commented Feb 18, 2015

the panicking modules have int types that need to change and are tied to lang items. It's all unstable though.

@GuillaumeGomez
Member

@brson: Not just the panicking. The entire libstd needs to be clean too.

@alexcrichton alexcrichton added a commit to alexcrichton/rust that referenced this issue Feb 19, 2015
@alexcrichton alexcrichton rollup merge of #22485: pnkfelix/fsk-int-uint-audit 9aee389
@Gankro
Contributor
Gankro commented Feb 20, 2015

Doing std/num/..

@Manishearth Manishearth added a commit to Manishearth/rust that referenced this issue Feb 21, 2015
@Manishearth Manishearth Rollup merge of #22600 - brson:num, r=Gankro
 * count_ones/zeros, trailing_ones/zeros return u32, not usize
* rotate_left/right take u32, not usize
* RADIX, MANTISSA_DIGITS, DIGITS, BITS, BYTES are u32, not usize

Doesn't touch pow because there's another PR for it.

cc rust-lang#22240

r? @Gankro
24a3f4c
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this issue Feb 21, 2015
@Manishearth Manishearth Rollup merge of #22600 - brson:num, r=Gankro
 * count_ones/zeros, trailing_ones/zeros return u32, not usize
* rotate_left/right take u32, not usize
* RADIX, MANTISSA_DIGITS, DIGITS, BITS, BYTES are u32, not usize

Doesn't touch pow because there's another PR for it.

cc rust-lang#22240

r? @Gankro
84891b9
@vojtechkral
Contributor

May I take the remaining libstd/thread_local/.. ? EDIT: done...

@vojtechkral vojtechkral added a commit to vojtechkral/rust that referenced this issue Feb 23, 2015
@vojtechkral vojtechkral Integer audit in `libstd/thread_local/*`, part of #22240 e5e76e9
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this issue Feb 24, 2015
@Manishearth Manishearth Rollup merge of #22728 - vojtechkral:int-audit-thread-local, r=alexcr…
…ichton

 Integer audit in `libstd/thread_local/*`, part of #22240
8c8bf3b
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this issue Feb 24, 2015
@Manishearth Manishearth Rollup merge of #22728 - vojtechkral:int-audit-thread-local, r=alexcr…
…ichton

 Integer audit in `libstd/thread_local/*`, part of #22240
3668324
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this issue Feb 24, 2015
@Manishearth Manishearth Rollup merge of #22728 - vojtechkral:int-audit-thread-local, r=alexcr…
…ichton

 Integer audit in `libstd/thread_local/*`, part of #22240
9b7c749
@bors bors added a commit that referenced this issue Feb 27, 2015
@bors bors Auto merge of #22510 - GuillaumeGomez:audit-integer-libstd-thread, r=…
…alexcrichton

Part of #22240.
54449c5
@bors bors added a commit that referenced this issue Feb 27, 2015
@bors bors Auto merge of #22504 - GuillaumeGomez:audit-integer-libcore, r=alexcr…
…ichton

Part of #22240.
222e372
@bors bors added a commit that referenced this issue Feb 27, 2015
@bors bors Auto merge of #22504 - GuillaumeGomez:audit-integer-libcore, r=alexcr…
…ichton

Part of #22240.
19c294a
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this issue Feb 28, 2015
@Manishearth Manishearth Rollup merge of #22504 - GuillaumeGomez:audit-integer-libcore, r=alex…
…crichton

 Part of #22240.
2875484
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this issue Feb 28, 2015
@Manishearth Manishearth Rollup merge of #22510 - GuillaumeGomez:audit-integer-libstd-thread, …
…r=alexcrichton

 Part of #22240.
0865131
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this issue Mar 1, 2015
@Manishearth Manishearth Rollup merge of #22504 - GuillaumeGomez:audit-integer-libcore, r=Mani…
…shearth

 Part of #22240.
fb19cd7
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this issue Mar 2, 2015
@Manishearth Manishearth Rollup merge of #22510 - GuillaumeGomez:audit-integer-libstd-thread, …
…r=alexcrichton

 Part of #22240.
d2b453a
@bors bors added a commit that referenced this issue Mar 2, 2015
@bors bors Auto merge of #22510 - GuillaumeGomez:audit-integer-libstd-thread, r=…
…alexcrichton

Part of #22240.
1cc8b6e
@bors bors added a commit that referenced this issue Mar 3, 2015
@bors bors Auto merge of #22600 - brson:num, r=Gankro
* count_ones/zeros, trailing_ones/zeros return u32, not usize
* rotate_left/right take u32, not usize
* RADIX, MANTISSA_DIGITS, DIGITS, BITS, BYTES are u32, not usize

Doesn't touch pow because there's another PR for it.

cc #22240

r? @Gankro
5457eab
@nikomatsakis
Contributor

Seems like it's time for someone to just finish all that remains in one fell swoop.

@GuillaumeGomez
Member

Everything has been taken already, no ?

@vojtechkral
Contributor

@GuillaumeGomez Looks like it.

@nikomatsakis
Contributor

@GuillaumeGomez yes I suppose you are correct, but there are several with no listed PR, all assigned to you or @vojtechkral, can you guys attest that those are in the pipeline now? I can't quite tell from all the various citations appearing everywhere.

@GuillaumeGomez
Member

All attributed to me have been merged.

@vojtechkral
Contributor

Mine too.

@nikomatsakis
Contributor

Great, thanks to you both!

@lfairy lfairy referenced this issue in lfairy/maud Oct 18, 2016
Merged

Add complicated Maud benchmark #52

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment