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

Replace deprecated integer suffixes #21378

Closed
wants to merge 1 commit into from
Closed

Replace deprecated integer suffixes #21378

wants to merge 1 commit into from

Conversation

alfiedotwtf
Copy link
Contributor

In the following example:

fn main() { 
    println!("{:?}", 42is);
    println!("{:?}", 42us);
} 

The output is:

42i
42u

This should be outputting the following instead:

42is
42us

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@alexcrichton
Copy link
Member

I think that there's definitely a number of tests which exercise this behavior, can you be sure to run the test suite and make sure there are no test failures as well?

@alfiedotwtf
Copy link
Contributor Author

Sure, will do once I'm connected to power.

(side note - I was always under the impression that Travis-CI ran the tests before allowing changes to be approved?)

@steveklabnik
Copy link
Member

Travis only runs 'make tidy'

@alfiedotwtf
Copy link
Contributor Author

@alexcrichton sorry. I'm an idiot - completely forgot about tests. I'll remember next time.

@steveklabnik ahh gotcha!

Tests have been updated and all pass, except for 4 Valgrind tests. @Kimundi on IRC said it could be something local on my machine doing this, so if anyone can check it out that would be great.

@alfiedotwtf
Copy link
Contributor Author

Travis doesn't like this because "line longer than 100 chars". Is this a config bug or enforced rule in Travis rather than a bug in my commit?

@alfiedotwtf
Copy link
Contributor Author

Got help from irc. I'm splitting the line since line limits are enforced (TIL). Just compiling and re-running make check now.

@alfiedotwtf
Copy link
Contributor Author

Ignore that last commit. My local is screwy.

@alfiedotwtf
Copy link
Contributor Author

Awesome...

Not sure that happened with that last commit. Anyhow, 5cc6a06 looks good apart from the 4 Valgrind errors. Also, to get around the 100 char limit, I cheated a bit with src/test/run-fail/assert-eq-macro-panic.rs

@alexcrichton
Copy link
Member

@bors: r+ 5cc6a06

Thanks!

@alfiedotwtf
Copy link
Contributor Author

Last night, it built from clean and all the tests passed (except Valgrind tests). This morning on a difference machine (same commit) it won't build for me as lots of assertions are failing. Can you cancel @bors including this change until I've sorted this out please.

Sorry for the inconvenience.

@alexcrichton
Copy link
Member

@bors: r-

Feel free to just ping this whenever you've got the tests passing again.

@alfiedotwtf
Copy link
Contributor Author

@alexcrichton Ok... so "make clean; make check" done. Everything passed except the same four Valgrind tests.

(I mustn't had done a make clean last night before submitting. I'll remember next time!)

@alfiedotwtf
Copy link
Contributor Author

Had to redo because make tidy was failing (over 100 chars), but make check was happy? Maybe check should include tidy?

hrmm.. more phantom commits e.g. e7f2280.

Ok, wait till I give the thumbs up. Doing a make clean, check, then tidy now.

@alexcrichton
Copy link
Member

Looks like there still may be some extra commits, perhaps a rebase is in order?

@alfiedotwtf
Copy link
Contributor Author

@alexcrichton It was weird because I rebased onto master...

Anyway, make clean, make check and make tidy. All good besides Valgrind. Thumbs up to 13b800d.

This has been a good learning experience for me. Sorry about giving you guys the run around!

@alexcrichton
Copy link
Member

@bors: r+ 13b800d

Thanks!

@tbu-
Copy link
Contributor

tbu- commented Jan 21, 2015

@alexcrichton This moves into the wrong direction, a recently accepted RFC says that the fmt::Show output of integer types should not include the integer suffix. Could you r- this?

@alfiedotwtf
Copy link
Contributor Author

@tbu- Sorry, didn't see that RFC.

So just to confirm, this is for all integer types and not just for is and us?

format!("{:?} {:?} {:?}", 42i32, 43is, 44us) == "42 43 44"

@tbu-
Copy link
Contributor

tbu- commented Jan 21, 2015

@Alfie Yes, all integer suffixes are to be omitted.

@alfiedotwtf
Copy link
Contributor Author

@tbu- cool, thanks for the clarification. Will update this PR.

@alexcrichton
Copy link
Member

Ah indeed @tbu-, I ended up taking care of this as part of #21457, so I'll just close this in favor of that. Thanks again @Alfie!

@alfiedotwtf alfiedotwtf deleted the println-suffix branch February 3, 2015 11:38
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