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

collections: impl Deref for Vec/String #18443

Merged
merged 1 commit into from
Oct 31, 2014

Conversation

alexcrichton
Copy link
Member

This commit adds the following impls:

impl<T> Deref<[T]> for Vec<T>
impl<T> DerefMut<[T]> for Vec<T>
impl Deref<str> for String

This commit also removes all duplicated inherent methods from vectors and
strings as implementations will now silently call through to the slice
implementation. Some breakage occurred at std and beneath due to inherent
methods removed in favor of those in the slice traits and std doesn't use its
own prelude,

cc #18424

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@@ -58,7 +58,7 @@ impl<'a,T> IntoMaybeOwnedVector<'a,T> for &'a [T] {
impl<'a,T> MaybeOwnedVector<'a,T> {
pub fn iter(&'a self) -> slice::Items<'a,T> {
match self {
&Growable(ref v) => v.iter(),
&Growable(ref v) => v.as_slice().iter(),
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this was require to avoid an obscure ICE somewhere in rustc. I wasn't able to quite track it down just yet, but I figured that the benefit of landing these trait impls would be much greater!

Copy link
Contributor

Choose a reason for hiding this comment

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

Worth adding a FIXME or issue?

Copy link
Member

Choose a reason for hiding this comment

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

What kind of ICE? I have a nasty one in a much larger changeset, and this could be helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah sorry I don't have the precise one offhand, but talking with @nikomatsakis it sounded like he was running into a very similar ICE (in this crate even!) and was fixing it, so I think this may fix itself soon.

The ICE was in ty::type_contents with a lookup in a map failing, and the context was somewhere in the bowels of trans (the immediate caller was expr_use_visitor::consume_expr. With the fix probably coming soon though I may just leave this for now.

Copy link

Choose a reason for hiding this comment

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

Sounds like #17810

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, it looked precisely like that, thanks @jakub-!

@alexcrichton alexcrichton mentioned this pull request Oct 30, 2014
24 tasks
@reem
Copy link
Contributor

reem commented Oct 30, 2014

:D

This commit adds the following impls:

    impl<T> Deref<[T]> for Vec<T>
    impl<T> DerefMut<[T]> for Vec<T>
    impl Deref<str> for String

This commit also removes all duplicated inherent methods from vectors and
strings as implementations will now silently call through to the slice
implementation. Some breakage occurred at std and beneath due to inherent
methods removed in favor of those in the slice traits and std doesn't use its
own prelude,

cc rust-lang#18424
@seanmonstar
Copy link
Contributor

\o/

@japaric
Copy link
Member

japaric commented Oct 30, 2014

@alexcrichton It's possible to use &*string to get an &str (in the right context) using this PR?

@alexcrichton
Copy link
Member Author

It should be, yes! I believe we're recommending string[] over &*string though as it's less jarring from a first perspective.

@japaric
Copy link
Member

japaric commented Oct 30, 2014

I believe we're recommending string[] over &*string though as it's less jarring from a first perspective.

I don't know the full story of feature gates, 1.0 and the stability channels, but it just occurred to me that people could end up using &*string instead because it's not feature-gated.

@reem
Copy link
Contributor

reem commented Oct 30, 2014

I actually strongly prefer &*string because it reads as "borrow the insides of this thing", which is clearer than "operator [] gets you a slice".

@seanmonstar
Copy link
Contributor

Coming from python, string[] is more immediately clear what it does. 🤷

@reem
Copy link
Contributor

reem commented Oct 30, 2014

From Python I might expect string[] to behave like string[:], which would make a sort of copy.

On Thu, Oct 30, 2014 at 10:30 AM, Sean McArthur [email protected]
wrote:

Coming from python, string[] is more immediately clear what it does. 🤷

Reply to this email directly or view it on GitHub:
#18443 (comment)

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Oct 31, 2014
@bors bors merged commit 8e9f8f9 into rust-lang:master Oct 31, 2014
@alexcrichton alexcrichton deleted the deref-vec-and-string branch October 31, 2014 05:05
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.

9 participants