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 a contains method to VecDeque and LinkedList #1552

Conversation

Projects
None yet
7 participants
@LukasKalbertodt
Copy link
Contributor

LukasKalbertodt commented Mar 22, 2016

Summary: add a contains method to VecDeque and LinkedList that checks if the collection contains a given item.

Rendered

@sfackler sfackler self-assigned this Mar 22, 2016

@sfackler sfackler added the T-libs label Mar 22, 2016

@main--

This comment has been minimized.

Copy link

main-- commented Mar 22, 2016

  • also add BinaryHeap::contains, since it could be convenient for some use cases, too

It's appropriate for most collections, so I'd suggest adding it to Iterator (with the obvious default impl self.any(|e| e == item)). The common usecase foo.iter().contains(bar) is a little bit longer but more generic.

@LukasKalbertodt

This comment has been minimized.

Copy link
Contributor Author

LukasKalbertodt commented Mar 22, 2016

adding it to Iterator

While it definitely makes sense to do so, we already have an inconsistency here: there already is a contains (or contains_key) method for [T], *Set, *Map and (more or less) Vec. Only VecDeque and LinkedList are lacking it. Therefore this argument still applies:

  • programmers that are used to call contains on a Vec are confused by the non-existence of the method for VecDeque or LinkedList

I also want to avoid creating many solutions for one programming task. I can already imagine confused students, asking about the difference of slice.contains(...) and slice.iter().contains(...). Or asking why you can write vec.contains(...), but need to write vec_deq.iter().contains(...).

I'm not overly happy with the current system due to inconsistencies, but I think adding contains to the remaining collections would make std more consistent. Also adding it to Iterator may be a good idea...

@ticki

This comment has been minimized.

Copy link
Contributor

ticki commented Mar 22, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 24, 2016

🔔 This RFC is now entering its week-long final comment period 🔔

Note, though, that the libs team also figured that small RFCs like this which already fit existing conventions are also appropriate to just send a PR :)

@mattico

This comment has been minimized.

Copy link

mattico commented Mar 30, 2016

With #1434 merged, if this gets merged we'll have .contains() for everything that it makes sense for except iterators (unless I've missed something). I think that's a good argument for implementing .contains() for iterators as well.

vec.contains(3); // works
hash_table.contains(3); // works
(0..100).contains(3); // works
"onomatopoeia".contains("e"); // works
vec.iter().map(|x| x*3).contains(3); // doesn't work?

Certainly there's opportunity for someone to unnecessarily .iter() something which natively implements .contains(), subtley reducing performance. I think it would be more likely that someone would be surprised that Iterator is missing this obvious (to me at least) method.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 31, 2016

The libs team discussed this RFC during triage today and the decision was to merge. Thanks again for the RFC @LukasKalbertodt!

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 31, 2016

Tracking issue: rust-lang/rust#32630

@alexcrichton alexcrichton merged commit f39ec21 into rust-lang:master Mar 31, 2016

@LukasKalbertodt LukasKalbertodt deleted the LukasKalbertodt:rfc-contains-method-for-various-collections branch Aug 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.