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

Use for instead of while in ascii.rs tests #21862

Merged
merged 1 commit into from
Feb 10, 2015
Merged

Use for instead of while in ascii.rs tests #21862

merged 1 commit into from
Feb 10, 2015

Conversation

iKevinY
Copy link
Contributor

@iKevinY iKevinY commented Feb 2, 2015

While working on this PR, I also noticed that all of the tests iterate through values up to 500. Just out of curiosity, was the choice of 500 arbitrary? I was thinking that it might make more sense to iterate up to 127 due to ASCII's character range (or if non-ASCII characters are being tested intentionally, either 255 or 511 for a nice 2^n - 1 value).

@rust-highfive
Copy link
Contributor

r? @brson

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

@brson
Copy link
Contributor

brson commented Feb 9, 2015

@bors: r+ 00d1 thanks!

I don't know off hand the significance of the numbers here.

@bors
Copy link
Collaborator

bors commented Feb 9, 2015

⌛ Testing commit 00d1873 with merge 5368ab1...

@bors
Copy link
Collaborator

bors commented Feb 9, 2015

💔 Test failed - auto-win-64-nopt-t

@iKevinY
Copy link
Contributor Author

iKevinY commented Feb 9, 2015

💔 Test failed - auto-win-64-nopt-t

Gah. :( Though since this hasn't been merged in yet, I'm still curious about the 500 magic number. The loops seem to me like they're just testing ASCII values (the assert_eq! checks preceding the loops seem to cover Unicode characters). Do you think it be okay for me to push another commit to this PR changing the 501s to 128s? (Unless of course there actually is a significance behind it.)

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Collaborator

bors commented Feb 9, 2015

⌛ Testing commit 00d1873 with merge 4668f51...

@bors
Copy link
Collaborator

bors commented Feb 9, 2015

💔 Test failed - auto-mac-64-opt

@alexcrichton
Copy link
Member

@bors: retry

bors added a commit that referenced this pull request Feb 9, 2015
I also noticed that all of the tests iterate through values up to 500. Just out of curiosity, was the choice of 500 arbitrary? I was thinking that it might make more sense to iterate up to 127 due to ASCII's character range (or if non-ASCII characters are being tested intentionally, either 255 or 511 for a nice `2^n - 1` value).
@bors
Copy link
Collaborator

bors commented Feb 9, 2015

⌛ Testing commit 00d1873 with merge d57a571...

@bors
Copy link
Collaborator

bors commented Feb 9, 2015

💔 Test failed - auto-win-64-nopt-t

@iKevinY
Copy link
Contributor Author

iKevinY commented Feb 9, 2015

Are the build fails related to the changes I made in my PR? There seems to be an awful lot of fails. :/

@alexcrichton
Copy link
Member

@bors: retry

Our automation is a ... little flaky sometimes :)

@iKevinY
Copy link
Contributor Author

iKevinY commented Feb 10, 2015

@alexcrichton Ah, I see; just wanted to make sure that I didn't break anything! Is a specific cause for the automation failure known? (Could the build infrastructure be improved or are there no practical fixes?)

@bors
Copy link
Collaborator

bors commented Feb 10, 2015

⌛ Testing commit 00d1873 with merge ec280bc...

@bors
Copy link
Collaborator

bors commented Feb 10, 2015

💔 Test failed - auto-win-64-nopt-t

@alexcrichton
Copy link
Member

@bors: retry rollup

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 10, 2015
I also noticed that all of the tests iterate through values up to 500. Just out of curiosity, was the choice of 500 arbitrary? I was thinking that it might make more sense to iterate up to 127 due to ASCII's character range (or if non-ASCII characters are being tested intentionally, either 255 or 511 for a nice `2^n - 1` value).
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Feb 10, 2015
@bors bors merged commit 00d1873 into rust-lang:master Feb 10, 2015
@iKevinY iKevinY deleted the libstd-ascii-tests branch February 10, 2015 23:41
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.

5 participants