Give SliceIndex impls a test suite of girth befitting the implementation by ExpHP · Pull Request #50010 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation15 Commits8 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

ExpHP

So one day I was writing something in my codebase that basically amounted to impl SliceIndex for (Bound<usize>, Bound<usize>), and I said to myself:

Boy, gee, golly! I never realized bounds checking was so tricky!

At some point when I had around 60 lines of tests for it, I decided to go see how the standard library does it to see if I missed any edge cases. ...That's when I discovered that libcore only had about 40 lines of tests for slicing altogether, and none of them even used ..=.

This PR includes:

@rust-highfive

r? @bluss

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

@ExpHP ExpHP changed the titleGive IndexSlice a test suite of girth befitting its implementation (and fix a unicode validation bug) Give IndexSlice a test suite of girth befitting its implementation (and fix a UTF8 boundary check)

Apr 16, 2018

@ExpHP ExpHP changed the titleGive IndexSlice a test suite of girth befitting its implementation (and fix a UTF8 boundary check) Give SliceIndex impls a test suite of girth befitting the implementation (and fix a UTF8 boundary check)

Apr 16, 2018

@rust-highfive

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log. 

Resolving deltas: 100% (605067/605067), completed with 4761 local objects.
---
[00:00:44] configure: rust.quiet-tests     := True
---
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:642: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:651: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:660: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:669: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:678: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:687: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:696: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:705: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:706: line longer than 100 chars
[00:04:45] tidy error: /checkout/src/liballoc/tests/str.rs:707: line longer than 100 chars
[00:04:46] some tidy checks failed
[00:04:46]
[00:04:46]
[00:04:46] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:46] expected success, got: exit code: 1
[00:04:46]
[00:04:46]
[00:04:46] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:46] Build completed unsuccessfully in 0:01:53
[00:04:46] Makefile:79: recipe for target 'tidy' failed
[00:04:46] make: *** [tidy] Error 1
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time🔚0258ba80:start=1523916453831324825,finish=1523916453837939675,duration=6614850
travis_fold🔚after_failure.2
travis_fold:start:after_failure.3
travis_time:start:03250e15
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time🔚03250e15:start=1523916453843528410,finish=1523916453849607489,duration=6079079
travis_fold🔚after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1f619d66
$ dmesg | grep -i kill
[   10.250665] init: failsafe main process (1092) killed by TERM signal

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@ExpHP

On second thought I think I'm going to pull the bug fix into a separate PR to make it easier to backport to beta

@ExpHP ExpHP mentioned this pull request

Apr 18, 2018

@ExpHP

Note: After #50039 merges, this will need to be rebased/force-pushed (I couldn't cherry-pick the commit from this PR).

When I do so I will likely:

so any review might want to hold off until then.

bors added a commit that referenced this pull request

Apr 21, 2018

@bors

smaller PR just to fix #50002

I pulled this out of #50010 to make it easier to backport to beta if necessary, considering that inclusive range syntax is stabilizing soon (?).

It fixes a bug in <str>::index_mut with (..=end) ranges (#50002), which prior to this fix was not only unusable but also UB in the cases where it "worked" (it gave improperly truncated UTF-8).

(not that I can imagine why anybody would use <str>::index_mut... but I'm not here to judge)

@bors

☔ The latest upstream changes (presumably #50039) made this pull request unmergeable. Please resolve the merge conflicts.

@pietroalbini

Ping from triage @ExpHP! #50039 was merged, this needs the rebase.

@ExpHP

Oops, sorry! I did something locally, but then started having second thoughts due to how uneven the PR makes this part of the test suite appear, and chickened out. I guess I should follow through and just get it back up here, to let others be the judge of that.

(also, my local branch currently has the cardinal sin of a later commit that broadly changes decisions made in an earlier commit, but it seemed too difficult to clean up due to intervening commits, so I'm afraid I might have to leave it like that. Blechh)

@ExpHP

@ExpHP

A previous PR fixed one method that was legitimately buggy; this cleans up the rest to be less diverse, mirroring the corresponding impls on [T] to the greatest extent possible without introducing any unnecessary UTF-8 boundary checks at 0.

@ExpHP

@ExpHP

GitHub users: I think you can add ?w=1 to the url for a vastly cleaner whitespace-ignoring diff

@ExpHP

mn lines of implementation deserves mn lines of tests

@ExpHP

@ExpHP

@ExpHP

@ExpHP

rebased, force-pushed, and put into correct chronological order using black magic.

Notes:

@kennytm kennytm added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

May 1, 2018

@shepmaster

Ping from triage, @bluss! Will you have time to review this in the near future?

@pietroalbini pietroalbini added the T-libs-api

Relevant to the library API team, which will review and decide on the PR/issue.

label

May 10, 2018

@pietroalbini

Ping from triage! This PR needs a review, can @bluss or someone else from @rust-lang/libs review this?

@alexcrichton

Egad sorry for the delay @ExpHP! Tihs all looks great to me, thanks!

@bors: r+

@bors

📌 Commit f1d7b45 has been approved by alexcrichton

@bors bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

May 10, 2018

@ExpHP ExpHP changed the titleGive SliceIndex impls a test suite of girth befitting the implementation (and fix a UTF8 boundary check) Give SliceIndex impls a test suite of girth befitting the implementation

May 10, 2018

alexcrichton added a commit to alexcrichton/rust that referenced this pull request

May 10, 2018

@alexcrichton

Give SliceIndex impls a test suite of girth befitting the implementation (and fix a UTF8 boundary check)

So one day I was writing something in my codebase that basically amounted to impl SliceIndex for (Bound<usize>, Bound<usize>), and I said to myself:

Boy, gee, golly! I never realized bounds checking was so tricky!

At some point when I had around 60 lines of tests for it, I decided to go see how the standard library does it to see if I missed any edge cases. ...That's when I discovered that libcore only had about 40 lines of tests for slicing altogether, and none of them even used ..=.

This PR includes:

@bors

⌛ Testing commit f1d7b45 with merge 5f566f2ae20052cc0fdb384120ee56ab4ff32b0d...

@alexcrichton

@bors bors added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

May 10, 2018

@ExpHP

Makes sense. The test was present before this PR, but my PR adds a to_owned() (hidden in the assert_range_eq! macro) that might be pushing it just over the limit.

bors added a commit that referenced this pull request

May 10, 2018

@bors

Rollup of 18 pull requests

Successful merges:

Failed merges:

@ExpHP ExpHP mentioned this pull request

Mar 26, 2020

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-libs-api

Relevant to the library API team, which will review and decide on the PR/issue.