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.

Sign up for GitHub

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

Optimize strip_prefix and strip_suffix with str patterns #69784

Merged
merged 1 commit into from
Mar 31, 2020
Merged

Optimize strip_prefix and strip_suffix with str patterns #69784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/liballoc/string.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1849,6 +1849,21 @@ impl<'a, 'b> Pattern<'a> for &'b String {
fn is_prefix_of(self, haystack: &'a str) -> bool {
self[..].is_prefix_of(haystack)
}

#[inline]
fn strip_prefix_of(self, haystack: &'a str) -> Option<&'a str> {
self[..].strip_prefix_of(haystack)
}

#[inline]
fn is_suffix_of(self, haystack: &'a str) -> bool {
self[..].is_suffix_of(haystack)
}

#[inline]
fn strip_suffix_of(self, haystack: &'a str) -> Option<&'a str> {
self[..].strip_suffix_of(haystack)
}
}

#[stable(feature = "rust1", since = "1.0.0")]
Expand Down
37 changes: 7 additions & 30 deletions src/libcore/str/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
#![stable(feature = "rust1", since = "1.0.0")]

use self::pattern::Pattern;
use self::pattern::{DoubleEndedSearcher, ReverseSearcher, SearchStep, Searcher};
use self::pattern::{DoubleEndedSearcher, ReverseSearcher, Searcher};

use crate::char;
use crate::fmt::{self, Write};
Expand Down Expand Up @@ -3986,26 +3986,15 @@ impl str {
/// ```
/// #![feature(str_strip)]
///
/// assert_eq!("foobar".strip_prefix("foo"), Some("bar"));
/// assert_eq!("foobar".strip_prefix("bar"), None);
/// assert_eq!("foo:bar".strip_prefix("foo:"), Some("bar"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering - do these changes improve the test cases or are they simply to make the docs more readable? Or some other thing? I assume it's not because the previous tests failed with the new implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They improve the test cases to account for a bug that I introduced while rewriting these methods that was not properly caught by the old test cases. The old test cases had the defect that they split the string exactly in half. My buggy implementation for strip_suffix sliced from s[pat.len()..] when it needed to slice from s[s.len() - pat.len()..], which was not caught by any of the previous test cases.

/// assert_eq!("foo:bar".strip_prefix("bar"), None);
/// assert_eq!("foofoo".strip_prefix("foo"), Some("foo"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be updated to foo:foo? I guess this partly depends on the comment above :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent of this test case, as best as I can tell, is to check that strip_prefix(P) only strips one prefix from a string like PPPP, so adding the colon would change what this test is intended to do.

/// ```
#[must_use = "this returns the remaining substring as a new slice, \
without modifying the original"]
#[unstable(feature = "str_strip", reason = "newly added", issue = "67302")]
pub fn strip_prefix<'a, P: Pattern<'a>>(&'a self, prefix: P) -> Option<&'a str> {
let mut matcher = prefix.into_searcher(self);
if let SearchStep::Match(start, len) = matcher.next() {
debug_assert_eq!(
start, 0,
"The first search step from Searcher \
must include the first character"
);
// SAFETY: `Searcher` is known to return valid indices.
unsafe { Some(self.get_unchecked(len..)) }
} else {
None
}
prefix.strip_prefix_of(self)
}

/// Returns a string slice with the suffix removed.
Expand All @@ -4020,8 +4009,8 @@ impl str {
///
/// ```
/// #![feature(str_strip)]
/// assert_eq!("barfoo".strip_suffix("foo"), Some("bar"));
/// assert_eq!("barfoo".strip_suffix("bar"), None);
/// assert_eq!("bar:foo".strip_suffix(":foo"), Some("bar"));
/// assert_eq!("bar:foo".strip_suffix("bar"), None);
/// assert_eq!("foofoo".strip_suffix("foo"), Some("foo"));
/// ```
#[must_use = "this returns the remaining substring as a new slice, \
Expand All @@ -4032,19 +4021,7 @@ impl str {
P: Pattern<'a>,
<P as Pattern<'a>>::Searcher: ReverseSearcher<'a>,
{
let mut matcher = suffix.into_searcher(self);
if let SearchStep::Match(start, end) = matcher.next_back() {
debug_assert_eq!(
end,
self.len(),
"The first search step from ReverseSearcher \
must include the last character"
);
// SAFETY: `Searcher` is known to return valid indices.
unsafe { Some(self.get_unchecked(..start)) }
} else {
None
}
suffix.strip_suffix_of(self)
}

/// Returns a string slice with all suffixes that match a pattern
Expand Down
85 changes: 85 additions & 0 deletions src/libcore/str/pattern.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,22 @@ pub trait Pattern<'a>: Sized {
matches!(self.into_searcher(haystack).next(), SearchStep::Match(0, _))
}

/// Removes the pattern from the front of haystack, if it matches.
#[inline]
fn strip_prefix_of(self, haystack: &'a str) -> Option<&'a str> {
if let SearchStep::Match(start, len) = self.into_searcher(haystack).next() {
debug_assert_eq!(
start, 0,
"The first search step from Searcher \
must include the first character"
);
// SAFETY: `Searcher` is known to return valid indices.
unsafe { Some(haystack.get_unchecked(len..)) }
} else {
None
}
}

/// Checks whether the pattern matches at the back of the haystack
#[inline]
fn is_suffix_of(self, haystack: &'a str) -> bool
Expand All @@ -55,6 +71,26 @@ pub trait Pattern<'a>: Sized {
{
matches!(self.into_searcher(haystack).next_back(), SearchStep::Match(_, j) if haystack.len() == j)
}

/// Removes the pattern from the back of haystack, if it matches.
#[inline]
fn strip_suffix_of(self, haystack: &'a str) -> Option<&'a str>
where
Self::Searcher: ReverseSearcher<'a>,
{
if let SearchStep::Match(start, end) = self.into_searcher(haystack).next_back() {
debug_assert_eq!(
end,
haystack.len(),
"The first search step from ReverseSearcher \
must include the last character"
);
// SAFETY: `Searcher` is known to return valid indices.
unsafe { Some(haystack.get_unchecked(..start)) }
} else {
None
}
}
}

// Searcher
Expand Down Expand Up @@ -448,13 +484,26 @@ impl<'a> Pattern<'a> for char {
self.encode_utf8(&mut [0u8; 4]).is_prefix_of(haystack)
}

#[inline]
fn strip_prefix_of(self, haystack: &'a str) -> Option<&'a str> {
self.encode_utf8(&mut [0u8; 4]).strip_prefix_of(haystack)
}

#[inline]
fn is_suffix_of(self, haystack: &'a str) -> bool
where
Self::Searcher: ReverseSearcher<'a>,
{
self.encode_utf8(&mut [0u8; 4]).is_suffix_of(haystack)
}

#[inline]
fn strip_suffix_of(self, haystack: &'a str) -> Option<&'a str>
where
Self::Searcher: ReverseSearcher<'a>,
{
self.encode_utf8(&mut [0u8; 4]).strip_suffix_of(haystack)
}
}

/////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -569,13 +618,26 @@ macro_rules! pattern_methods {
($pmap)(self).is_prefix_of(haystack)
}

#[inline]
fn strip_prefix_of(self, haystack: &'a str) -> Option<&'a str> {
($pmap)(self).strip_prefix_of(haystack)
}

#[inline]
fn is_suffix_of(self, haystack: &'a str) -> bool
where
$t: ReverseSearcher<'a>,
{
($pmap)(self).is_suffix_of(haystack)
}

#[inline]
fn strip_suffix_of(self, haystack: &'a str) -> Option<&'a str>
where
$t: ReverseSearcher<'a>,
{
($pmap)(self).strip_suffix_of(haystack)
}
};
}

Expand Down Expand Up @@ -715,11 +777,34 @@ impl<'a, 'b> Pattern<'a> for &'b str {
haystack.as_bytes().starts_with(self.as_bytes())
}

/// Removes the pattern from the front of haystack, if it matches.
#[inline]
fn strip_prefix_of(self, haystack: &'a str) -> Option<&'a str> {
if self.is_prefix_of(haystack) {
// SAFETY: prefix was just verified to exist.
unsafe { Some(haystack.get_unchecked(self.as_bytes().len()..)) }
} else {
None
}
}

/// Checks whether the pattern matches at the back of the haystack
#[inline]
fn is_suffix_of(self, haystack: &'a str) -> bool {
haystack.as_bytes().ends_with(self.as_bytes())
}

/// Removes the pattern from the back of haystack, if it matches.
#[inline]
fn strip_suffix_of(self, haystack: &'a str) -> Option<&'a str> {
if self.is_suffix_of(haystack) {
let i = haystack.len() - self.as_bytes().len();
// SAFETY: suffix was just verified to exist.
unsafe { Some(haystack.get_unchecked(..i)) }
} else {
None
}
}
}

/////////////////////////////////////////////////////////////////////////////
Expand Down