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

str: common logic for replace & replacen #56464

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@ljedrz
Contributor

ljedrz commented Dec 3, 2018

The logic of str::replace and str::replacen is almost identical - we can use a common helper function. The initial capacity of the resulting string was different (0 vs. 32), but I propose a different approach: use the same capacity as in the original string; this will neither reallocate too often (or not at all if the substitute's length is the same as the substituted bit's), nor will it overallocate for short strings. It will also be an exact fit in case of no pattern hits.

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Dec 3, 2018

r? @kennytm

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

@kennytm

This comment has been minimized.

Member

kennytm commented Dec 3, 2018

I'm going to rewrite these functions for #56345, would you mind if I just incorporate these changes directly into the new PR?

@ljedrz

This comment has been minimized.

Contributor

ljedrz commented Dec 3, 2018

Sure, I don't mind at all 👌.

@kennytm

This comment has been minimized.

Member

kennytm commented Dec 3, 2018

OK I'll close this when that PR is submitted.

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Dec 3, 2018

The job x86_64-gnu-llvm-5.0 of 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.
travis_time:end:070486b0:start=1543852283535146173,finish=1543852370569137316,duration=87033991143
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:52:55] 
[00:52:55] running 120 tests
[00:52:58] i..ii...iii..iiii.....i...i..........i..iii.............i.....i.....ii...i..i.ii...............i..ii 100/120
[00:52:59] ..ii.i.....iiii.....
[00:52:59] 
[00:52:59]  finished in 3.260
[00:52:59] travis_fold:end:test_codegen

---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:53:12] 
[00:53:12] running 118 tests
[00:53:35] .iiiii...i.....i..i...i..i.i..i.i..i.....i..i....i..........iiii.........i.i....i...i.......ii.i.i.i 100/118
[00:53:38] ......iii.i.....ii
[00:53:38] 
[00:53:38]  finished in 26.194
[00:53:38] travis_fold:end:test_debuginfo

---
[01:01:15]    Compiling rand_core v0.2.1
[01:01:15]    Compiling libc v0.2.43
[01:01:16]    Compiling rand v0.5.5
[01:01:20]    Compiling alloc v0.0.0 (/checkout/src/liballoc)
[01:01:25] error: function is never used: `replace_helper`
[01:01:25]     |
[01:01:25]     |
[01:01:25] 609 | fn replace_helper<'a, P: Pattern<'a>>(s: &'a str, pat: P, to: &str, lim: Option<usize>) -> String {
[01:01:25]     |
[01:01:25]     = note: `-D dead-code` implied by `-D warnings`
[01:01:25] 
[01:01:25] error: aborting due to previous error
[01:01:25] error: aborting due to previous error
[01:01:25] 
[01:01:25] error: Could not compile `alloc`.
[01:01:25] warning: build failed, waiting for other jobs to finish...
[01:02:29] error: build failed
[01:02:29] 
[01:02:29] 
[01:02:29] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "alloc" "--" "--quiet"
[01:02:29] 
[01:02:29] 
[01:02:29] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:02:29] Build completed unsuccessfully in 0:19:40
[01:02:29] Build completed unsuccessfully in 0:19:40
[01:02:29] make: *** [check] Error 1
[01:02:29] Makefile:58: recipe for target 'check' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:00239916
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Mon Dec  3 16:55:30 UTC 2018

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)

@@ -622,3 +605,16 @@ impl str {
pub unsafe fn from_boxed_utf8_unchecked(v: Box<[u8]>) -> Box<str> {
Box::from_raw(Box::into_raw(v) as *mut str)
}
fn replace_helper<'a, P: Pattern<'a>>(s: &'a str, pat: P, to: &str, lim: Option<usize>) -> String {

This comment has been minimized.

@Centril

Centril Dec 3, 2018

Contributor

A comment would be nice... :)

let mut last_end = 0;
let limit = if let Some(limit) = lim { limit } else { s.len() };
for (start, part) in s.match_indices(pat).take(limit) {
result.push_str(unsafe { s.get_unchecked(last_end..start) });

This comment has been minimized.

@Centril

Centril Dec 3, 2018

Contributor

And a comment explaining why this is safe as well...

result.push_str(to);
last_end = start + part.len();
}
result.push_str(unsafe { s.get_unchecked(last_end..s.len()) });

This comment has been minimized.

@Centril

Centril Dec 3, 2018

Contributor

and here...

@Dylan-DPC

This comment has been minimized.

Member

Dylan-DPC commented Dec 10, 2018

@ljedrz can you update this PR with the changes requested above?

@ljedrz

This comment has been minimized.

Contributor

ljedrz commented Dec 10, 2018

@Dylan-DPC actually @kennytm is interested in intercepting this change (see #56464 (comment)); that's why I'm not updating this PR - it will be closed when it becomes a part of #56345.

@Dylan-DPC

This comment has been minimized.

Member

Dylan-DPC commented Dec 10, 2018

Thanks. Marking it as blocked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment