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

docs: moving from int and uint to us for array access in examples #21277

Closed
wants to merge 1 commit into from
Closed

Conversation

alfiedotwtf
Copy link
Contributor

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

@alexcrichton
Copy link
Member

I think that as an introduction to Rust that this would actually omit as many suffixes as possible (instead of being explicit), would it be possible to just delete all suffixes?

@alfiedotwtf
Copy link
Contributor Author

I'm conflicted here. I agree that you would want to aim for the intro to be as easy as possible.

So there's two commits here...

With 4172150, although removing suffixes would be easier for a newbie, it's probably better to drum it in from the start that you probably want to be explicit when indexing by using usize. This way, it's habit forming and a plus if they miss the reasoning in later texts.

As for 48a58dd, that was just to silence the compiler warnings when a newbie were to copy-paste the examples and compile them. Having a screen full of warnings on their first attempt to compile some Rust from the official intro may actually scare them off!

I'm happy to remove all suffixes, but I just thought you may want to hear my reasoning :)

@steveklabnik
Copy link
Member

In general, as few suffixes as possible.

This way, it's habit forming

Right, but a bad habit. 😄

Which warnings does this make?

@alfiedotwtf
Copy link
Contributor Author

Right, but a bad habit.

Fair enough :) Although I was under the impression that you didn't want the default (no-suffix, signed) for array indexing so wanted to make usize habit forming. If I'm right, should indexing into an array cause a type check in the compiler and complain when it's not usize?

As for the warnings, one of the examples "let mut numbers = vec![1i, 2i, 3i];" generates 31 lines of warnings. This could definitely scare off a newbie.

/root/tmp/test/src/main.rs:2:26: 2:28 warning: the `i` suffix on integers is deprecated; use `is` or one of the fixed-sized suffixes                                                                                                                                                                                           
/root/tmp/test/src/main.rs:2   let mut numbers = vec![1i, 2i, 3i];
                                                  ^~  
<std macros>:1:1: 9:54 note: in expansion of vec!
/root/tmp/test/src/main.rs:2:21: 2:38 note: expansion site
/root/tmp/test/src/main.rs:2:26: 2:28 help: add #![feature(int_uint)] to the crate attributes to silence this warning
/root/tmp/test/src/main.rs:2   let mut numbers = vec![1i, 2i, 3i];
                                                  ^~  
<std macros>:1:1: 9:54 note: in expansion of vec!
/root/tmp/test/src/main.rs:2:21: 2:38 note: expansion site
/root/tmp/test/src/main.rs:2:30: 2:32 warning: the `i` suffix on integers is deprecated; use `is` or one of the fixed-sized suffixes
/root/tmp/test/src/main.rs:2   let mut numbers = vec![1i, 2i, 3i];
                                                      ^~  
<std macros>:1:1: 9:54 note: in expansion of vec!
/root/tmp/test/src/main.rs:2:21: 2:38 note: expansion site
/root/tmp/test/src/main.rs:2:30: 2:32 help: add #![feature(int_uint)] to the crate attributes to silence this warning
/root/tmp/test/src/main.rs:2   let mut numbers = vec![1i, 2i, 3i];
                                                      ^~  
<std macros>:1:1: 9:54 note: in expansion of vec!
/root/tmp/test/src/main.rs:2:21: 2:38 note: expansion site
/root/tmp/test/src/main.rs:2:34: 2:36 warning: the `i` suffix on integers is deprecated; use `is` or one of the fixed-sized suffixes
/root/tmp/test/src/main.rs:2   let mut numbers = vec![1i, 2i, 3i];
                                                          ^~  
<std macros>:1:1: 9:54 note: in expansion of vec!
/root/tmp/test/src/main.rs:2:21: 2:38 note: expansion site
/root/tmp/test/src/main.rs:2:34: 2:36 help: add #![feature(int_uint)] to the crate attributes to silence this warning
/root/tmp/test/src/main.rs:2   let mut numbers = vec![1i, 2i, 3i];
                                                          ^~  
<std macros>:1:1: 9:54 note: in expansion of vec!
/root/tmp/test/src/main.rs:2:21: 2:38 note: expansion site
/root/tmp/test/src/main.rs:2:7: 2:18 warning: variable does not need to be mutable, #[warn(unused_mut)] on by default
/root/tmp/test/src/main.rs:2   let mut numbers = vec![1i, 2i, 3i];
                               ^~~~~~~~~~~

@steveklabnik
Copy link
Member

So this is now out of date. Can you rebase? I think we removed them since. :)

@alfiedotwtf
Copy link
Contributor Author

That's what I love... Rust moving fast - it's a feature :)

@alexcrichton
Copy link
Member

Can the remaining us suffixes be removed? I would expect inference to drive them to already be us as they're used for indexing, but I could be wrong!

@alfiedotwtf
Copy link
Contributor Author

@alexcrichton this was interesting experiment! I ran the following to check who's assumption was right:

fn main() { 
    let v = [5,6,7];
    for j in 0..3 {
        println!("{}", v[j]);
        println!("{:?}", j);
    }
} 

I was under the assumption that this was going to be 'is' as 'int' used to be the default but now moved to isize. I was completely off here as integers without a suffix instead default to i32! @alexcrichton expected the output to be 'us' because of the type inference, but this actually outputs the following:

5
0u
6
1u
7
2u

I think what's happening here is that println!() is actually wrong because it's spitting out 'u' and not 'us'! I've submitted edde04b to fix this.

Sorry guys, I was completely wrong! This change should be dropped :)

@alexcrichton
Copy link
Member

Thanks for the investigation @Alfie!

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.

4 participants