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

Add str::split_ascii_whitespace. #49987

Merged
merged 1 commit into from
Jun 28, 2018

Conversation

clarfonthey
Copy link
Contributor

As mentioned in #48656.

While str::split_whitespace now works in libcore, it still makes sense to offer this method, considering how it is still more performant in cases where only ASCII is necessary.

@rust-highfive
Copy link
Collaborator

r? @bluss

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 15, 2018
@rust-highfive
Copy link
Collaborator

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.
[00:00:47] configure: rust.quiet-tests     := True
---
[00:03:09] error[E0277]: the trait bound `for<'r> str::IsAsciiWhitespace: ops::function::FnMut<(&'r u8,)>` is not satisfied
[00:03:09]     --> libcore/str/mod.rs:2635:5
[00:03:09]      |
[00:03:09] 2635 |     inner: Map<Filter<SliceSplit<'a, u8, IsAsciiWhitespace>, IsNotEmpty>, BytesToStr>,
[00:03:09]      |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `for<'r> ops::function::FnMut<(&'r u8,)>` is not implemented for `str::IsAsciiWhitespace`
[00:03:09]      |
[00:03:09] note: required by `slice::Split`
[00:03:09]     --> libcore/slice/mod.rs:1655:1
[00:03:09]      |
[00:03:09] 1655 | pub struct Split<'a, T:'a, P> where P: FnMut(&T) -> bool {
---
[00:03:12] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:03:12] expected success, got: exit code: 101
[00:03:12] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1085:9
[00:03:12] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:03:12] travis_fold:end:stage0-std
[00:03:12] travis_time:end:stage0-std:start=1523825587941191947,finish=1523825599238220358,duration=11297028411
[00:03:12] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:12] Build completed unsuccessfully in 0:00:12
[00:03:12] Makefile:79: recipe for target 'tidy' failed
[00:03:12] 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:end:0e665b96:start=1523825599726098956,finish=1523825599732533241,duration=6434285
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:15481c42
$ 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:end:15481c42:start=1523825599738037634,finish=1523825599743859096,duration=5821462
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:05d172d8
$ dmesg | grep -i kill
[   10.219692] init: failsafe main process (1096) 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)

@rust-highfive
Copy link
Collaborator

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.
[00:00:48] configure: rust.quiet-tests     := True
---
[00:03:11] error[E0277]: the trait bound `for<'r> str::IsAsciiWhitespace: ops::function::FnMut<(&'r u8,)>` is not satisfied
[00:03:11]     --> libcore/str/mod.rs:2635:5
[00:03:11]      |
[00:03:11] 2635 |     inner: Map<Filter<SliceSplit<'a, u8, IsAsciiWhitespace>, IsNotEmpty>, BytesToStr>,
[00:03:11]      |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `for<'r> ops::function::FnMut<(&'r u8,)>` is not implemented for `str::IsAsciiWhitespace`
[00:03:11]      |
[00:03:11] note: required by `slice::Split`
[00:03:11]     --> libcore/slice/mod.rs:1655:1
[00:03:11]      |
[00:03:11] 1655 | pub struct Split<'a, T:'a, P> where P: FnMut(&T) -> bool {
---
[00:03:14] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:03:14] expected success, got: exit code: 101
[00:03:14] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1085:9
[00:03:14] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:03:14] travis_fold:end:stage0-std
[00:03:14] travis_time:end:stage0-std:start=1523826888830573051,finish=1523826900542284021,duration=11711710970
[00:03:14] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:14] Build completed unsuccessfully in 0:00:12
[00:03:14] Makefile:79: recipe for target 'tidy' failed
[00:03:14] 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:end:1e050dde:start=1523826901630436855,finish=1523826901637034378,duration=6597523
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:012eeec5
$ 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:end:012eeec5:start=1523826901642919086,finish=1523826901648654846,duration=5735760
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1a23cb7b
$ dmesg | grep -i kill
[   10.398374] 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)

@jcarres-mdsol
Copy link

Does this need to be standard lib?
If anything it could be split_whitespace_ascii so always shows next to the other whitespace method

@clarfonthey
Copy link
Contributor Author

clarfonthey commented Apr 16, 2018

@jcarres-mdsol Methods are sorted in the order they're defined, and I'd really rather not have is_ascii_whitespace but split_whitespace_ascii.

@bors
Copy link
Contributor

bors commented Apr 22, 2018

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

@pietroalbini pietroalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 30, 2018
@pietroalbini
Copy link
Member

Ping from triage @clarcharr! There are conflicts that needs to be resolved, and the tests are currently failing.

@clarfonthey
Copy link
Contributor Author

Rebased! Sorry it took so long.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@kennytm kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 6, 2018
@clarfonthey
Copy link
Contributor Author

(This is ready for review @bluss)

@pietroalbini pietroalbini 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 9, 2018
@TimNN
Copy link
Contributor

TimNN commented Jun 12, 2018

Ping from triage, @bluss / @rust-lang/libs, this PR needs your review.

@pietroalbini
Copy link
Member

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

@pietroalbini
Copy link
Member

Ping from triage! It's more than 20 days since this PR needed a review. Can @bluss or someone else from @rust-lang/libs review this?


impl<'a> FnMut<(&'a [u8], )> for BytesToStr {
#[inline]
extern "rust-call" fn call_mut(&mut self, arg: (&'a [u8], )) -> &'a str {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really should be an unsafe fn, but unfortunately that’s not supported by the Fn* traits. It’s not essential though since this is a private type.

Maybe renaming the type to UncheckedBytesToStr and adding a comment that it is unsafe to use? Feel free to submit another PR for this, I’ll r+ this one now so that it doesn’t bitrot again.

@SimonSapin
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 27, 2018

📌 Commit 0ce3ea2c968a64b382aed9ee26f59c1db158eb2c has been approved by SimonSapin

@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 Jun 27, 2018
@bors
Copy link
Contributor

bors commented Jun 27, 2018

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout split_ascii_whitespace (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self split_ascii_whitespace --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
Auto-merging src/libcore/str/mod.rs
CONFLICT (file/directory): There is a directory with name src/libbacktrace in heads/homu-tmp. Adding src/libbacktrace as src/libbacktrace~HEAD
Auto-merging src/liballoc/str.rs
Auto-merging src/liballoc/lib.rs
Automatic merge failed; fix conflicts and then commit the result.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 27, 2018
@pietroalbini
Copy link
Member

@clarcharr could you please rebase on top of the latest master, even if you don't see any conflict? Thanks!

@clarfonthey
Copy link
Contributor Author

Will do!

Also I'll make the change @SimonSapin suggested. IMHO these should really be using impl Fn to have the same effect but not require a separate type, although I'll do that in a separate PR.

@SimonSapin
Copy link
Contributor

Can impl Fn syntax be used inside a struct definition?

@clarfonthey
Copy link
Contributor Author

@SimonSapin Through some form of abstract type, yes. Although this doesn't appear to be implemented yet so that'll have to happen later.

@clarfonthey
Copy link
Contributor Author

Also, rebased.

@SimonSapin
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 27, 2018

📌 Commit b5cee02 has been approved by SimonSapin

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 27, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Jun 27, 2018
…=SimonSapin

Add str::split_ascii_whitespace.

As mentioned in rust-lang#48656.

While `str::split_whitespace` now works in `libcore`, it still makes sense to offer this method, considering how it is still more performant in cases where only ASCII is necessary.
bors added a commit that referenced this pull request Jun 27, 2018
Rollup of 7 pull requests

Successful merges:

 - #49987 (Add str::split_ascii_whitespace.)
 - #50342 (Document round-off error in `.mod_euc()`-method, see issue #50179)
 - #51658 (Only do sanity check with debug assertions on)
 - #51799 (Lower case some feature gate error messages)
 - #51800 (Add a compiletest header for edition)
 - #51824 (Fix the error reference for LocalKey::try_with)
 - #51842 (Document that Layout::from_size_align does not allow align=0)

Failed merges:

r? @ghost
@bors bors merged commit b5cee02 into rust-lang:master Jun 28, 2018
@clarfonthey clarfonthey deleted the split_ascii_whitespace branch July 23, 2018 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants