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

Remove the need for allocation from is_utf8_string. #368

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hovinen
Copy link
Collaborator

@hovinen hovinen commented Mar 23, 2024

Previously, IsEncodedStringMatcher converted its actual value input to a Vec and constructed a String out of that in order to match it. This is because using a string slice caused problems with the borrow checker as described in #323.

The fundamental problem is that the type against which the inner matcher is matching is a reference whose lifetime must be named in the trait bound:

impl<ActualT: AsRef<[u8]> + Debug, InnerMatcherT> Matcher
    for IsEncodedStringMatcher<ActualT, InnerMatcherT>
where
    InnerMatcherT: Matcher<ActualT = &'what str>,
                                       ^^^^ What goes here???

One option is just to remove the reference:

impl<ActualT: AsRef<[u8]> + Debug, InnerMatcherT> Matcher
    for IsEncodedStringMatcher<ActualT, InnerMatcherT>
where
    InnerMatcherT: Matcher<ActualT = str>,

This doesn't work when the inner matcher is eq, since one would then be comparing an str and a string slice:

verify_that!("A string".as_bytes(), is_utf8_string(eq("A string"))) // Compile error: cannot compare str and &str

However, this problem does not occur with StrMatcher:

verify_that!("A string".as_bytes(), is_utf8_string(constains_substring("A string"))) // Passes

So this also introduces an easy way to obtain a StrMatcher to check string equality:

verify_that!("A string".as_bytes(), is_utf8_string(eq_str("A string"))) // Passes

This is slightly inconvenient, since one must use a different matcher to compare string equality in this case. But it is well-documented and not too inconvenient to use. So it should be okay.

The introduction of eq_str also allows fixing a longstanding limitation of the matcher property! -- that it could not match on properties returning string slices. This PR therefore also updates the documentation accordingly and adds an appropriate test.

@hovinen hovinen force-pushed the no-alloc-on-is-utf8-string branch 2 times, most recently from 4a1540b to 799ff7c Compare March 28, 2024 12:43
Previously, `IsEncodedStringMatcher` converted its actual value input to
a `Vec` and constructed a `String` out of that in order to match it.
This is because using a string slice caused problems with the borrow
checker as described in #323.

The fundamental problem is that the type against which the inner matcher
is matching is a reference whose lifetime must be named in the trait
bound:

```
impl<ActualT: AsRef<[u8]> + Debug, InnerMatcherT> Matcher
    for IsEncodedStringMatcher<ActualT, InnerMatcherT>
where
    InnerMatcherT: Matcher<ActualT = &'what str>,
                                       ^^^^ What goes here???
```

One option is just to remove the reference:

```
impl<ActualT: AsRef<[u8]> + Debug, InnerMatcherT> Matcher
    for IsEncodedStringMatcher<ActualT, InnerMatcherT>
where
    InnerMatcherT: Matcher<ActualT = str>,
```

This doesn't work when the inner matcher is `eq`, since one would then
be comparing an `str` and a string slice:

```
verify_that!("A string".as_bytes(), is_utf8_string(eq("A string"))) // Compile error: cannot compare str and &str
```

However, this problem does not occur with `StrMatcher`:

```
verify_that!("A string".as_bytes(), is_utf8_string(constains_substring("A string"))) // Passes
```

So this also introduces an easy way to obtain a `StrMatcher` to check
string equality:

```
verify_that!("A string".as_bytes(), is_utf8_string(eq_str("A string"))) // Passes
```

This is slightly inconvenient, since one must use a different matcher
to compare string equality in this case. But it is well-documented and
not too inconvenient to use. So it should be okay.

The introduction of `eq_str` also allows fixing a longstanding
limitation of the matcher `property!` -- that it could not match on
properties returning string slices. This PR therefore also updates the
documentation accordingly and adds an appropriate test.
@hovinen hovinen force-pushed the no-alloc-on-is-utf8-string branch from 799ff7c to f3bbdfc Compare March 28, 2024 13:17
@gribozavr
Copy link
Collaborator

@hovinen Thank you for the PR! This generally makes sense to me, however the PR has developed merge conflicts with the code at HEAD. Would you mind rebasing? Please reassign back to me when done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants