-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Give SliceIndex impls a test suite of girth befitting the implementation #50010
Conversation
r? @bluss (rust_highfive has picked a reviewer for you, use r? to override) |
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.
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 |
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 |
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. |
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)
☔ The latest upstream changes (presumably #50039) made this pull request unmergeable. Please resolve the merge conflicts. |
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) |
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.
GitHub users: I think you can add ?w=1 to the url for a vastly cleaner whitespace-ignoring diff
m*n lines of implementation deserves m*n lines of tests
rebased, force-pushed, and put into correct chronological order using black magic. Notes:
|
Ping from triage, @bluss! Will you have time to review this in the near future? |
Ping from triage! This PR needs a review, can @bluss or someone else from @rust-lang/libs review this? |
📌 Commit f1d7b45 has been approved by |
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: * **Literally the first appearance of the word `get_unchecked_mut` in any directory named `test` or `tests`.** * Likewise the first appearance of `get_mut` used with _any type of range argument_ in these directories. * Tests for the panics on overflow with `..=`. * I wanted to test on `[(); usize::MAX]` as well but that takes linear time in debug mode </3 * A horrible and ugly test-generating macro for the `should_panic` tests that increases the DRYness by a single order of magnitude (which IMO wasn't enough, but I didn't want to go any further and risk making the tests inaccessible to next guy). * Same stuff for str! * Actually, the existing `str` tests were pretty good. I just helped filled in the holes. * [A fix for the bug it caught](rust-lang#50002). (only one ~~sadly~~)
⌛ Testing commit f1d7b45 with merge 5f566f2ae20052cc0fdb384120ee56ab4ff32b0d... |
@bors: retry r- prioritizing rollup which includes this in it I think the |
Makes sense. The test was present before this PR, but my PR adds a |
Rollup of 18 pull requests Successful merges: - #49423 (Extend tests for RFC1598 (GAT)) - #50010 (Give SliceIndex impls a test suite of girth befitting the implementation (and fix a UTF8 boundary check)) - #50447 (Fix update-references for tests within subdirectories.) - #50514 (Pull in a wasm fix from LLVM upstream) - #50524 (Make DepGraph::previous_work_products immutable) - #50532 (Don't use Lock for heavily accessed CrateMetadata::cnum_map.) - #50538 ( Make CrateNum allocation more thread-safe. ) - #50564 (Inline `Span` methods.) - #50565 (Use SmallVec for DepNodeIndex within dep_graph.) - #50569 (Allow for specifying a linker plugin for cross-language LTO) - #50572 (Clarify in the docs that `mul_add` is not always faster.) - #50574 (add fn `into_inner(self) -> (Idx, Idx)` to RangeInclusive (#49022)) - #50575 (std: Avoid `ptr::copy` if unnecessary in `vec::Drain`) - #50588 (Move "See also" disambiguation links for primitive types to top) - #50590 (Fix tuple struct field spans) - #50591 (Restore RawVec::reserve* documentation) - #50598 (Remove unnecessary mutable borrow and resizing in DepGraph::serialize) - #50606 (Retry when downloading the Docker cache.) Failed merges: - #50161 (added missing implementation hint) - #50558 (Remove all reference to DepGraph::work_products)
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:
get_unchecked_mut
in any directory namedtest
ortests
.get_mut
used with any type of range argument in these directories...=
.[(); usize::MAX]
as well but that takes linear time in debug mode </3should_panic
tests that increases the DRYness by a single order of magnitude (which IMO wasn't enough, but I didn't want to go any further and risk making the tests inaccessible to next guy).str
tests were pretty good. I just helped filled in the holes.sadly)