Moving away from deprecated i/u suffixes in libcore #21511

alfiedotwtf · 2015-01-22T15:26:55Z

No description provided.

rust-highfive · 2015-01-22T15:27:11Z

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

alfiedotwtf · 2015-01-22T15:27:39Z

If everyone is happy with this change, I'll move forward with the rest. Otherwise, let me know what needs to be done and I'll give it a go.

Note: I was thinking of updating [u]int => [u]isize, but that should be a separate change.

eddyb · 2015-01-22T15:37:28Z

I started looking through these and realized commenting on each one would be pointless.
Most uses of integral suffixes outside of code examples in documentation seem to be unnecessary.
Do we have any style guidelines related to the use of suffixes in general? cc @aturon

alfiedotwtf · 2015-01-22T15:53:29Z

@eddyb I've tried looking for style on this but didn't find anything. The word on IRC today was to drop the suffix when it can be inferred, which I've done.

Are you sure you want to drop most suffixes? e.g.

let mut n = 0i;  // original: int
let mut n = 0is; // mine: isize
let mut n = 0;   // dropped: i32

I deliberately didn't want to change the datatypes like above, only move away from the deprecated suffixes.

eddyb · 2015-01-22T17:53:21Z

@Alfie the type only changes if it's unconstrained anywhere, which seems a bit suspicious. Do you have examples of this? I'll look through the diff again, I guess.

eddyb · 2015-01-22T18:00:30Z

src/libcore/fmt/mod.rs

@@ -846,7 +846,7 @@ macro_rules! tuple {
            fn fmt(&self, f: &mut Formatter) -> Result {
                try!(write!(f, "("));
                let ($(ref $name,)*) = *self;
-                let mut n = 0i;
+                let mut n = 0is;


First usage that needs annotation. Even then, this is used for 3 states, which would be only 2 if we were to special-case one-element tuples. Feels saner that way IMO.
We can do it without adding another case to the macro, too, and while removing the peel helper macro.
Something like $first, $($name),*.

eddyb · 2015-01-22T18:15:12Z

None of the 3 cases I found would actually cause a subtle change in semantics (the last one would error if 1 would default to a differently-sized type).
They are all in macros, and even then, the suffixes can be removed while cleaning up the way the macro works.
I ignored examples in doc comments because they are highly artificial, though some of them may work with less suffixes.

alfiedotwtf · 2015-01-23T00:30:49Z

@eddyb Ok, no problem. I've updated the patch:

- Suffixes have been dropped in code
- Suffixes updated from deprecated i/u in documentation

I'd like for the examples to be updated because it's going to be confusing in the future once i/u are no longer valid syntax.

eddyb · 2015-01-23T05:25:42Z

src/libcore/num/mod.rs

-                            'E' | 'e' if radix == 10 => 10u as $T,
-                            'P' | 'p' if radix == 16 => 2u as $T,
+                            'E' | 'e' if radix == 10 => 10.0 as $T,
+                            'P' | 'p' if radix == 16 => 2.0 as $T,


You don't need as $T at all, the type is either f32 or f64.

alfiedotwtf · 2015-01-23T06:01:12Z

@eddyb Thanks for that. I wasn't too sure about that one.

huonw · 2015-01-23T06:46:07Z

I think much of the documentation can have the suffixes dropped too: in most cases it doesn't matter if it's a i32 or isize or whatever, and the cases where it does matter it should infer (I would guess).

alfiedotwtf · 2015-01-23T07:07:41Z

@huonw No problem. Fixed.

eddyb · 2015-01-23T09:32:35Z

src/libcore/default.rs

@@ -150,13 +150,13 @@ default_impl! { (), () }
 default_impl! { bool, false }
 default_impl! { char, '\x00' }

-default_impl! { uint, 0u }
+default_impl! { uint, 0us }


All of these suffixed integers should be able to be just 0.

alfiedotwtf · 2015-01-23T10:07:09Z

@eddyb Thanks for the clarification. I wasn't too sure about that one. Fixed.

eddyb · 2015-01-23T10:27:11Z

Right now the only us/is suffixes added in the entire diff are in the transmute calls in the slice iterators - which could be replaced later with something pointer-based (e.g. &mut *(1 as *mut _)).
I am honestly surprised this is possible on such a scale. Great work, @Alfie!

alfiedotwtf · 2015-01-23T13:19:00Z

@eddyb Thanks for that. I wasn't sure what to replace it with, but that looks reasonable. Fixed.

huonw · 2015-01-24T11:01:11Z

@bors: r+ 4

Looks great!

bors · 2015-01-24T11:01:13Z

🙀 You have the wrong number! Please try again with 45b6d40.

huonw · 2015-01-24T11:16:21Z

@bors: r+ 45b6d40

(Drat, a single letter is too short. 😝 )

bors · 2015-01-24T11:46:38Z

⌛ Testing commit 45b6d40 with merge b64d9fe...

bors · 2015-01-24T13:04:38Z

💔 Test failed - auto-mac-32-opt

alfiedotwtf · 2015-01-24T13:18:12Z

Was that auto-mac-32-opt failure legit?

huonw · 2015-01-24T14:36:14Z

src/libcore/iter.rs

@@ -1138,7 +1138,7 @@ pub trait AdditiveIterator<A> {
    /// ```
    /// use std::iter::AdditiveIterator;
    ///
-    /// let a = [1i, 2, 3, 4, 5];
+    /// let a = [1, 2, 3, 4, 5];


I think this needs a suffix (e.g. 1i32).

huonw · 2015-01-24T14:36:55Z

@Alfie yeah, I think so.

alfiedotwtf · 2015-01-24T14:46:50Z

@huonw Thanks. Ok, will fix.

Can you just clear something up for me please. These are all in comments and not code, but it still failed?

Also, you said to make the following to an i32:

/// let a = [1i, 2, 3, 4, 5];
/// let a = [1, 2, 3, 4, 5];

I thought that if an integer doesn't have a suffix, it's typed as an i32 as a default? If so, why should this need an i32 suffix?

huonw · 2015-01-24T15:06:21Z

The rustdoc supports running code examples in doc-comments as tests and we run these tests on all crates to keep the docs up to date.

I'm not sure what's happening with those examples, but inference can be a little sensitive/less-than-perfect when there's closures around, at the moment.

alfiedotwtf · 2015-01-24T15:08:04Z

@huonw Thanks for the explanation.

huonw · 2015-01-25T00:02:53Z

@bors: r+ 42958d3

huonw · 2015-01-25T00:03:52Z

src/libcore/char.rs

@@ -258,7 +258,29 @@ impl CharExt for char {
    #[inline]
    #[unstable = "pending decision about Iterator/Writer/Reader"]
    fn encode_utf8(self, dst: &mut [u8]) -> Option<uint> {
-        encode_utf8_raw(self as u32, dst)
+        // Marked #[inline] to allow llvm optimizing it away


Hm, why was this changed?

alfiedotwtf · 2015-01-25T00:12:56Z

I got a conflict while rebasing onto master. I was sure I resolved it correctly. Fixing now :(

alfiedotwtf · 2015-01-25T00:18:40Z

Apologies. I resolved the conflict that the wrong way. Fixed.

huonw · 2015-01-25T00:19:56Z

@bors: r+ f67e747

It's not problem at all, rebases are tricky like that sometimes.

bors · 2015-01-25T00:49:42Z

⌛ Testing commit f67e747 with merge 70b13a7...

bors · 2015-01-25T03:24:52Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

rust-highfive assigned huonw Jan 22, 2015

eddyb reviewed Jan 22, 2015
View reviewed changes

alfiedotwtf changed the title ~~moving from i/u to is/us suffixes in libcore~~ Moving away from deprecated i/u suffixes in libcore Jan 23, 2015

eddyb reviewed Jan 23, 2015
View reviewed changes

huonw reviewed Jan 24, 2015
View reviewed changes

huonw reviewed Jan 25, 2015
View reviewed changes

Moving away from deprecated i/u suffixes in libcore

f67e747

bors added a commit that referenced this pull request Jan 25, 2015

Auto merge of #21511 - alfie:suffix-cleanup, r=huonw

70b13a7

bors merged commit f67e747 into rust-lang:master Jan 25, 2015

alfiedotwtf deleted the suffix-cleanup branch February 3, 2015 11:38

eddyb mentioned this pull request Feb 18, 2015

Remove redundant integer suffixes. #22501

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving away from deprecated i/u suffixes in libcore #21511

Moving away from deprecated i/u suffixes in libcore #21511

alfiedotwtf commented Jan 22, 2015

rust-highfive commented Jan 22, 2015

alfiedotwtf commented Jan 22, 2015

eddyb commented Jan 22, 2015

alfiedotwtf commented Jan 22, 2015

eddyb commented Jan 22, 2015

eddyb Jan 22, 2015

eddyb commented Jan 22, 2015

alfiedotwtf commented Jan 23, 2015

eddyb Jan 23, 2015

alfiedotwtf commented Jan 23, 2015

huonw commented Jan 23, 2015

alfiedotwtf commented Jan 23, 2015

eddyb Jan 23, 2015

alfiedotwtf commented Jan 23, 2015

eddyb commented Jan 23, 2015

alfiedotwtf commented Jan 23, 2015

huonw commented Jan 24, 2015

bors commented Jan 24, 2015

huonw commented Jan 24, 2015

bors commented Jan 24, 2015

bors commented Jan 24, 2015

alfiedotwtf commented Jan 24, 2015

huonw Jan 24, 2015

huonw commented Jan 24, 2015

alfiedotwtf commented Jan 24, 2015

huonw commented Jan 24, 2015

alfiedotwtf commented Jan 24, 2015

huonw commented Jan 25, 2015

huonw Jan 25, 2015

alfiedotwtf commented Jan 25, 2015

alfiedotwtf commented Jan 25, 2015

huonw commented Jan 25, 2015

bors commented Jan 25, 2015

bors commented Jan 25, 2015

Moving away from deprecated i/u suffixes in libcore #21511

Moving away from deprecated i/u suffixes in libcore #21511

Conversation

alfiedotwtf commented Jan 22, 2015

rust-highfive commented Jan 22, 2015

alfiedotwtf commented Jan 22, 2015

eddyb commented Jan 22, 2015

alfiedotwtf commented Jan 22, 2015

eddyb commented Jan 22, 2015

eddyb Jan 22, 2015

Choose a reason for hiding this comment

eddyb commented Jan 22, 2015

alfiedotwtf commented Jan 23, 2015

eddyb Jan 23, 2015

Choose a reason for hiding this comment

alfiedotwtf commented Jan 23, 2015

huonw commented Jan 23, 2015

alfiedotwtf commented Jan 23, 2015

eddyb Jan 23, 2015

Choose a reason for hiding this comment

alfiedotwtf commented Jan 23, 2015

eddyb commented Jan 23, 2015

alfiedotwtf commented Jan 23, 2015

huonw commented Jan 24, 2015

bors commented Jan 24, 2015

huonw commented Jan 24, 2015

bors commented Jan 24, 2015

bors commented Jan 24, 2015

alfiedotwtf commented Jan 24, 2015

huonw Jan 24, 2015

Choose a reason for hiding this comment

huonw commented Jan 24, 2015

alfiedotwtf commented Jan 24, 2015

huonw commented Jan 24, 2015

alfiedotwtf commented Jan 24, 2015

huonw commented Jan 25, 2015

huonw Jan 25, 2015

Choose a reason for hiding this comment

alfiedotwtf commented Jan 25, 2015

alfiedotwtf commented Jan 25, 2015

huonw commented Jan 25, 2015

bors commented Jan 25, 2015

bors commented Jan 25, 2015