Skip to content

Remove get method from InternedString #21505

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Feb 7, 2015
Merged

Remove get method from InternedString #21505

merged 14 commits into from
Feb 7, 2015

Conversation

GuillaumeGomez
Copy link
Member

It's in order to make the code more homogeneous.

@rust-highfive
Copy link
Contributor

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@GuillaumeGomez
Copy link
Member Author

No feedback ?

@GuillaumeGomez
Copy link
Member Author

r? @alexcrichton

@alexcrichton
Copy link
Member

Thanks! The Str trait currently isn't necessarily meant for implementations as it may not survive stabilization of the std::str module. The get method can probably be removed entirely in favor of the Deref implementation, however.

@GuillaumeGomez
Copy link
Member Author

Okay ! Then do I just remove the Str trait and the get method ?

@alexcrichton
Copy link
Member

Yeah that should work just fine.

@alexcrichton
Copy link
Member

@bors: r+ 4e53266

Feel free to ping the PR when it gets updated, sadly github doesn't send out notifications on a force push!

@GuillaumeGomez
Copy link
Member Author

My bad, I didn't think about that ! Thanks !

@bors
Copy link
Collaborator

bors commented Feb 2, 2015

⌛ Testing commit 4e53266 with merge 4f559b0...

@bors
Copy link
Collaborator

bors commented Feb 2, 2015

💔 Test failed - auto-mac-64-opt

@GuillaumeGomez
Copy link
Member Author

@alexcrichton: it should be ready now !

@alexcrichton
Copy link
Member

Thanks! In general though you should never need to call deref manually. If for example you have foo.deref().bar() you can actually just call it foo.bar(). If you have foo.deref() then you can replace it with &foo[] (in the case of slicing) or &*foo in the case of a raw pointer.

@GuillaumeGomez
Copy link
Member Author

I found this writing:

foo.deref()

beautifuler than this one:

&*foo

But I can change it if you prefer ! At least I'll rebase on the master to solve merge conflicts.

@alexcrichton
Copy link
Member

I think that deref coercions may be in a snapshot now so instead of foo.deref() I believe you can pass &foo (as opposed to &*foo)

@GuillaumeGomez
Copy link
Member Author

@alexcrichton: It's (finally) ready to be merged !

@alexcrichton
Copy link
Member

@bors: r+ d6bfb9a

@bors
Copy link
Collaborator

bors commented Feb 4, 2015

⌛ Testing commit d6bfb9a with merge 4495d7d...

@bors
Copy link
Collaborator

bors commented Feb 4, 2015

💔 Test failed - auto-linux-64-x-android-t

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Collaborator

bors commented Feb 4, 2015

⌛ Testing commit d6bfb9a with merge 2ba5a04...

@bors
Copy link
Collaborator

bors commented Feb 4, 2015

💔 Test failed - auto-mac-64-nopt-t

@alexcrichton
Copy link
Member

@GuillaumeGomez the most recent failure is the more relevant one.

Also, in light of #21916 you may want to replace as many &foo[] with &foo as possible to help prevent churn when [] is switched to [..].

@GuillaumeGomez
Copy link
Member Author

@alexcrichton: Ok ! Just like you said. I update my code and let's try again !

@GuillaumeGomez GuillaumeGomez changed the title Add Str trait to InternedString, set get method deprecated Remove get method from InternedString Feb 4, 2015
@GuillaumeGomez
Copy link
Member Author

Tests worked. @alexcrichton, r+ please ?

@alexcrichton
Copy link
Member

@GuillaumeGomez could you squash some of these commits together as well? Mainly just the "fix foo" commits which don't necessarily need to exist.

@GuillaumeGomez
librustdoc has been updated
a2e01c6 
Fixes run build error

Fix test failure

Fix tests' errors
@GuillaumeGomez
Copy link
Member Author

Done !

@alexcrichton
Copy link
Member

@bors: r+ a2e01c6

@bors
Copy link
Collaborator

bors commented Feb 6, 2015

⌛ Testing commit a2e01c6 with merge bfe1453...

@bors
Copy link
Collaborator

bors commented Feb 6, 2015

💔 Test failed - auto-linux-32-opt

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Collaborator

bors commented Feb 7, 2015

⌛ Testing commit a2e01c6 with merge 7ebf9bc...

bors added a commit that referenced this pull request Feb 7, 2015
@bors
Auto merge of #21505 - GuillaumeGomez:interned_string, r=alexcrichton
7ebf9bc 
It's in order to make the code more homogeneous.
@bors
Copy link
Collaborator

bors commented Feb 7, 2015

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

@bors bors merged commit a2e01c6 into rust-lang:master Feb 7, 2015
@GuillaumeGomez GuillaumeGomez deleted the interned_string branch February 7, 2015 11:42
renato-zannon added a commit to renato-zannon/racer that referenced this pull request Feb 8, 2015
@renato-zannon
token::InternedString no longer has a .get() method
cd75428 
See rust-lang/rust#21505
@renato-zannon renato-zannon mentioned this pull request Feb 8, 2015
Closed
@Potpourri Potpourri mentioned this pull request Feb 8, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@GuillaumeGomez @rust-highfive @alexcrichton @bors @sfackler