Update Pattern machinery to use more generic version of string like things - #161608
Update Pattern machinery to use more generic version of string like things#161608pacak wants to merge 17 commits into
Conversation
The std::sys::os_str::{Buf, Slice} types are only used within the std
crate and not actually exported. Whole `sys` module is private. They
don't need to be public. This might result in a better generated code,
but more importantly it avoids some compile errors down the line.
Firstly, combine functions and results lists into a single list with 'function => result' pairs. This makes it easier to match function with its result. Secondly, eliminate InRange step so that it's easier to notice series of matches or rejects. @pacak: I added a variant to test_stress_indices that matches stuff
Right now things are undertested and underspecified. Some of the library code would get in a loop if searcher starts returning empty rejects. And there's no tests for backwards multi byte char matchers. Pull request I'm reviving had a problem implementing that, so making sure it's tested before the actual code lands. Right now it is possible to break both tests (and user code) without breaking anything else in the test suite I think.
Surprisingly enough there's no rfind tests for multibyte needles, at least it is possible to break this test without breaking anything other test.
Add a Haystack trait describing something that can be searched in and make core::str::Pattern (and related types) generic on that trait. This will allow Pattern to be used for types other than str (most notably OsStr). This somewhat follows the Pattern API 2.0 design. While that design is apparently abandoned (?), it is somewhat helpful when going for patterns on OsStr, so I’m going with it unless someone tells me otherwise. ;) For now leave Pattern, Haystack et al in core::str::pattern. Since they are no longer str-specific, I’ll move them to core::pattern in future commit. This one leaves them in place to make the diff smaller. @pacak: I moved some (or all new) of the `P: Pattern<&'a str> constraints into where clause to keep things narrower: ``` pub fn foo<'a, P: Pattern<&'a str>>(&'a self, pat: P, ...) ... ``` to ``` pub fn replacen<'a, P>(&'a self, pat: P, ...) ... where P: Pattern<&'a str>, ``` Original code had indices in Haystack abstracted as an associated type Cursor. Replaced with usize - Cursor adds noise with not much value. Changed wording in 2-3 places - for example Searcher is generic over a few types so it makes more sense to talk about split points in general with utf8 split points as an example for `&str`.
Pattern is no longer str-specific, so move it from core::str::pattern module to a new core::pattern module. This introduces no changes in behaviour or implementation. Just moves stuff around and adjusts documentation.
Introduce core::pattern::Split and core::pattern::SplitN internal types which can be used to implement iterators splitting haystack into parts. Convert str’s Split-family of iterators to use them. In the future, more haystacks will use those internal types. Co-authored-by: Peter Jaszkowiak <p.jaszkow@gmail.com> @pacak: Fixed some typos, added a few `#[inline]`. Since there's no `H::Cursor` - I had to add `ctx: PhantomData<H>`.
This reverts commit 85cf233ced0d0fe02734c8a83b6d79ccc5432d06. Gone for now, I'll reimplement it later in str_bytes.rs, will confirm with the benchmarks included that the optimization still applies
Introduce core::pattern::EmptyNeedleSearcher internal type which implements logic for matching an empty pattern against a haystack. Convert core::str::pattern::StrSearcher to use it. In future more implementations will take advantage of it. Also adapt and rework TwoWayStrategy into an internal SearchResult trait which abstracts differences between Searcher’s next, next_match and next_rejects methods. It makes it simpler to write a single generic method implementing optimised versions of all those calls. @pacak: - Fixed a few typos. - There's no H::Cursor parameter so code gets a bit simplified. - Added a test to assert how TwoWaySearcher runs with EmptyNeedleSearcher
@pacak: - made more things const fn - there was a (copy-paste?) error in try_finish_byte_sequence so I added a test that checks try_next_code_point(_reverse) with some values, including invalid ones. - reworded a few comments (passive voice, etc) Also different comments: since former is public and later is private due to historical reasons. > This is different than [`next_code_point`] in that it doesn't assume > This is different than `next_code_point_reverse` in that it doesn't assume
Introduce a new core::str_bytes module with types and functions which handle string-like bytes slices. String-like means that they code treats UTF-8 byte sequences as characters within such slices but doesn't assume that the slices are well-formed. A `str` is trivially a bytes sequence that the module can handle but so is OsStr (which is WTF-8 on Windows and unstructured bytes on Unix). Move bunch of code (most notably implementation of the two-way string-matching algorithm) from core::str to core::str_bytes. Note that this likely introduces regression in some of the str function performance (since the new code cannot assume well-formed UTF-8). This is going to be rectified by following commit which will make it again possible for the code to assume bytes format. This is not done in this commit to keep it smaller. @pacak: - Added a few comments - tried to hide internal types from the diagnostic And then there's two different bugs where it would report matched areas as rejected. This broke str::trim_end_matches and who knows what else. Caught it thanks to tests in the previous commit. And one underflow bug on invalid input.
It works right now, but original implementation of the next commit breaks them with none of existing tests catching this regression.
Since core::str_bytes module cannot assume byte slices it deals with are well-formed UTF-8 (or even WTF-8), the code must be defensive and accept invalid sequences. This eliminates optimisations which would be otherwise possible. Introduce a `Flavour` trait which tags `Bytes` type with information about the byte sequence. For example, if a `Bytes` object is created from `&str` it’s tagged with `Utf8` flavour which gives the code freedom to assume data is well-formed UTF-8. This brings back all the optimisations removed in previous commit. @pacak: - removed IS_WTF8 associated constant - unused - fixed a bug related to multibyte reverse matching: `next_code_point_reverse` reads the input via Iterator::next_back, passing `bytes.iter().rev()` reverses it a second time. Not good.
I reverted `ByteNeedle` change earlier, time to add the same functionality back. `ByteSearcherState` is mostly copied from `CharSearcherState`, does a single ascii byte search. It is possible to do the dispatch inside of a CharSearcherState, but that makes it a bit slower.
pattern::find_str 4775.12ns/iter -> 2561.57ns/iter
pattern::rfind_str 5621.05ns/iter -> 2492.68ns/iter
|
cc @Amanieu, @folkertdev, @sayantn |
rustbot
added
S-waiting-on-review
|
r? @clarfonthey rustbot has assigned @clarfonthey. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
rustbot
added
has-merge-commits
Yes, I'll clean this up once stuff is merged. For now they are needed so I can split a chungus of a pull request into smaller pieces, while having each commit compiling with passing tests. |
| /// assert_eq!("abcdef_z".find(|ch| ch > 'd' && ch < 'y'), Some(4)); | ||
| /// assert_eq!("abcddd_z".find(|ch| ch > 'd' && ch < 'y'), None); | ||
| /// ``` | ||
| pub trait Pattern<H: Haystack>: Sized { |
There was a problem hiding this comment.
Just as a basic vibe-check: is there a reason why we want to do Pattern<&'a str> instead of just Pattern<str>?
Since I would imagine that it's okay to assume that all the types we do this for will be DST slices like OsStr, [u8], or str, and thus, this should be okay. I'm not a huge fan of the for<'a> Pattern<&'a str> bounds.
There was a problem hiding this comment.
As an aside, this may require making things like type Searcher become type Searcher<'a> instead, but I'd prefer that over the current approach.
There was a problem hiding this comment.
That's what original attempt at this change did and then there's this: rust-lang/libs-team#311
I'll give Pattern<str> a go. I think this might clean up some of the user facing bits.
There was a problem hiding this comment.
Btw, change to the Pattern is done here #161606
There was a problem hiding this comment.
Done, changes should be visible in #161606, going though remaining commits.
There was a problem hiding this comment.
Given this
pub trait Pattern<H: Haystack>: Sized
...
then Pattern<str> means Haystack must be implemented for str. Haystack comes with fn get_unchecked(&self, range: Range) -> &Self. Can implement for &str since we can create one.
Then we add pub struct Bytes<'a>(&'a [u8]);, later pub struct Bytes<'a, F>(&'a [u8], PhantomData<F>);. That cannot implement get_unchecked due to fat pointer shenanigans.
I guess I might get it working if I turn Bytes into an unsized type, similar to str or [u8]...
Before I depart on this quest - thoughts?
There was a problem hiding this comment.
Kind of works and looks somewhat better. On the other hand a hundred or so conflicts each commit. So it'll take a day or two.
% jj diff --tool :git | rg '<<< conflict' | wc
76 380 2035
There was a problem hiding this comment.
Yeah, we would require it to be a proper DST in these cases, so if you find a reason why it shouldn't be one, we can do otherwise. I'm just not expecting that to be the case.
|
☔ The latest upstream changes (presumably #161638) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
3 participants
(assuming prerequisite commits are merged) pattern machinery can be made with any string like things (
&OsStrspecifically), but right now it is mostly hardcoded to&str. Here we add a concept ofFlavor- utf8, wtf8 and unstructured indicating how paranoid we should be parsing it.This pull request reverts 0013230, leaving benchmarks from the same pull request in place, reimplements it later in terms of new code then piles up some optimizations on top so resulting code is as fast or faster than before.
This pull request also contains commits from #161589 #161592 #161596 #161604 #161606 and should be merged after all of those are merged in. I'll rebase it later. For review purposes - please ignore first 7 commits and start from the revert one.
This pull request is a part of the #160971 cinematic universe.