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 slice::contains_ref to supplement slice::contains #74044

Closed
wants to merge 3 commits into from

Conversation

poliorcetics
Copy link
Contributor

Fixes #42671.

This adds the last function proposed in #42671:

pub fn contains_ref<U>(&self, x: &U) -> bool where T: PartialEq<U>, U: ?Sized { /* ... */ }

I simply used self.iter().any(|e| e == x) as a first implementation, I don't know if there is a more optimised version possible.

I put an unstable flag, should I create a tracking issue for this ? I don't know if this is a "big enough" feature for it ?

@rustbot modify labels: A-collections,C-enhancement,T-doc,T-libs

@rustbot rustbot added A-collections Area: std::collections. C-enhancement Category: An issue proposing an enhancement or a PR with one. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 4, 2020
@rust-highfive
Copy link
Collaborator

r? @sfackler

(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 Jul 4, 2020
@poliorcetics poliorcetics changed the title First version of contains_ref Add slice::contains_ref to supplement slice::contains Jul 4, 2020
@Muirrum
Copy link
Member

Muirrum commented Jul 24, 2020

@sfackler This is a triage bump.

@bors
Copy link
Contributor

bors commented Jul 28, 2020

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

@Muirrum Muirrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2020
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 4, 2020
@Dylan-DPC-zz
Copy link

r? @scottmcm

@rust-highfive rust-highfive assigned scottmcm and unassigned sfackler Sep 4, 2020
Copy link
Member

@scottmcm scottmcm left a comment

Choose a reason for hiding this comment

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

Hmm, this is one of those things that's almost all API decisions, so while I have no technical issues with the code, I don't feel like this is something I can approve myself, even for unstable.

So ping @rust-lang/libs, as I didn't notice any comments from them about this approach in #42671, and marking waiting-on-team.

A handful of ponderings:

  • Is this the right name? [T]::contains also takes a ref, so it's not obvious to me that contains_ref is the right distinguisher.
  • Is the PartialEq<Other> approach the best one, given that it's a new method without the inference-breakage restrictions of the existing contains? For example, is there a way this can support simpler callers, like allowing .contains_bikeshed(0) instead of needing the caller to pass &0?

library/core/src/slice/mod.rs Outdated Show resolved Hide resolved
/// assert!(v.contains_ref("Hello"));
/// assert!(!v.contains_ref("Rust"));
/// ```
#[unstable(feature = "contains_ref", issue = "none")]
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remember it !

@scottmcm scottmcm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 5, 2020
@poliorcetics
Copy link
Contributor Author

Does this example does what you want @scottmcm ?

struct MyNum(u8);

struct MyOtherNum(u8);

impl PartialEq<u8> for MyNum {
    fn eq(&self, other: &u8) -> bool {
        self.0 == *other
    }
}

impl PartialEq<MyOtherNum> for MyNum {
    fn eq(&self, other: &MyOtherNum) -> bool {
        self.0 == other.0
    }
}

impl PartialEq<&u8> for MyNum {
    fn eq(&self, other: &&u8) -> bool {
        self.0 == **other
    }
}

impl PartialEq<&MyOtherNum> for MyNum {
    fn eq(&self, other: &&MyOtherNum) -> bool {
        self.0 == other.0
    }
}

// Same functionality as the `contains_ref` method in the PR.
fn search<T, U>(sl: &[T], other: U) -> bool
                            //   ^ takes `U` instead of `&U`
where
    T: PartialEq<U>,
    // No `U: ?Sized` here
{
    sl.iter().any(|e| e == &other)
                            // ^ uses a `&` here
}

fn main() {
    let v = [String::from("Hello"), String::from("world")];
    assert!(search(&v, "Hello"));
    assert!(!search(&v, "Rust"));

    let v = [0, 2];
    assert!(search(&v, 0));
    assert!(!search(&v, 1));

    let v = [MyNum(0), MyNum(2)];
    assert!(search(&v, 0));
    assert!(!search(&v, 1));
    assert!(search(&v, &0));
    assert!(!search(&v, &1));
    assert!(search(&v, MyOtherNum(0)));
    assert!(!search(&v, MyOtherNum(2)));
    assert!(search(&v, &MyOtherNum(0)));
    assert!(!search(&v, &MyOtherNum(2)));
}

It needs the implementation for u8 and &u8 which I find bothering because it means a function cannot call search like this:

fn do_something<T, U>(sl: &[T], other: &U)
where
    T: PartialEq<U>,
{
    search(sl, other);
            // ^^^^^ expected type parameter `U`, found `&U`
            // but `U` is not `Copy`.

    // ...
}

@withoutboats
Copy link
Contributor

I don't think the name contains_ref captures the distinction from contains that is relevant here.

@poliorcetics
Copy link
Contributor Author

I agree with that, I just don't have any idea for a better name.

@LukasKalbertodt
Copy link
Member

I also think the name is problematic, but also can't think of a better one. Unless we find a nice name, I would prefer not to add this API: v.iter().any(|x| x == "needle") is already pretty concise and is only marginally less readable/understandable than v.contains("needled") in my opinion. I don't think it's worth expanding the API in this case.

@poliorcetics
Copy link
Contributor Author

Feel free to close if there is no idea for a better name.

It may be worth closing the original issue too in this case since the v.iter().any(|x| x == "needle") workaround is already explained in the documentation for contains on stable.

@crlf0710
Copy link
Member

crlf0710 commented Oct 8, 2020

Triage: What's the status here? Should this be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Area: std::collections. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). 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.

Add Vec::contains_ref which is worse for inference but less restrictive than Vec::contains