move `get_ident` and `get_name` free function onto the `Ident` and `Name` types #27234

oli-obk · 2015-07-23T12:57:10Z

this has quite some fallout. but also made lots of stuff more readable imo

[breaking-change] for plugin authors

rust-highfive · 2015-07-23T12:57:27Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @huonw (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

eddyb · 2015-07-23T13:12:01Z

Do we need Ident::get_ident? Also, Name::get_name could be just Name::as_str - the current implementation for the latter is unsound anyway (the interning tables are not eternal nor thread-safe).

oli-obk · 2015-07-23T13:25:30Z

src/librustc_back/svh.rs

@@ -313,10 +313,10 @@ mod svh_visitor {
    // Ad-hoc overloading between Ident and Name to their intern table lookups.
    trait InternKey { fn get_content(self) -> token::InternedString; }


@eddyb: I could implement something like this... maybe From<Ident/Name> for InternedString?

alexcrichton · 2015-07-23T15:05:54Z

cc @rust-lang/compiler

nikomatsakis · 2015-07-23T19:17:07Z

looks nice to me.

bors · 2015-07-26T10:41:12Z

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

oli-obk · 2015-07-27T07:06:23Z

rebased and applied @eddyb's suggestion to make as_str sound and remove get_ident

caused additional fallout obviously.

oli-obk · 2015-07-27T07:10:48Z

src/libsyntax/ast.rs

@@ -166,12 +159,15 @@ pub const ILLEGAL_CTXT : SyntaxContext = 1;
           RustcEncodable, RustcDecodable, Clone, Copy)]
 pub struct Name(pub u32);

+impl<T: AsRef<str>> PartialEq<T> for Name {


I figured this would make life a lot easier. sadly I cannot implement the commutative version of this.

The alternative was often &*ident.name.as_str() == "text" when it's now ident.name == "text"

eddyb · 2015-07-27T15:07:39Z

@oli-obk Apart from the .into() calls (which I'm unsure about), it looks really good to me, great work!

huonw · 2015-07-27T20:14:22Z

r? @eddyb (seems to have it under control)

oli-obk · 2015-07-28T10:39:31Z

removed into-impl, replaced all calls by to_str, rebased and squashed

eddyb · 2015-07-28T10:49:44Z

@bors r+

bors · 2015-07-28T10:49:45Z

📌 Commit abc3317 has been approved by eddyb

bors · 2015-07-28T12:05:40Z

⌛ Testing commit abc3317 with merge 1232a4d...

bors · 2015-07-28T12:53:28Z

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

oli-obk · 2015-07-28T16:09:25Z

sorry about that. I had sporadic failures in test_process_mask and thought the rest went through...

now "make check" passed without a single complaint.

eddyb · 2015-07-28T16:21:39Z

@bors r+

bors · 2015-07-28T16:21:39Z

📌 Commit 00a5e66 has been approved by eddyb

bors · 2015-07-28T21:14:30Z

⌛ Testing commit 00a5e66 with merge 55ede7e...

this has quite some fallout. but also made lots of stuff more readable imo [breaking-change] for plugin authors

bors · 2015-07-28T22:51:52Z

☀️ 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-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

See rust-lang/rust#27234

rust-highfive assigned huonw Jul 23, 2015

oli-obk reviewed Jul 23, 2015
View reviewed changes

oli-obk force-pushed the move_get_name_get_ident_to_impl branch 2 times, most recently from 410abfa to cc476e9 Compare July 24, 2015 12:44

oli-obk force-pushed the move_get_name_get_ident_to_impl branch from d6d1e51 to 2f6d5dd Compare July 27, 2015 06:28

oli-obk reviewed Jul 27, 2015
View reviewed changes

rust-highfive assigned eddyb and unassigned huonw Jul 27, 2015

oli-obk force-pushed the move_get_name_get_ident_to_impl branch 3 times, most recently from cfd225b to abc3317 Compare July 28, 2015 10:38

remove get_ident and get_name, make as_str sound

00a5e66

oli-obk force-pushed the move_get_name_get_ident_to_impl branch from abc3317 to 00a5e66 Compare July 28, 2015 16:07

bors added a commit that referenced this pull request Jul 28, 2015

Auto merge of #27234 - oli-obk:move_get_name_get_ident_to_impl, r=eddyb

55ede7e

this has quite some fallout. but also made lots of stuff more readable imo [breaking-change] for plugin authors

bors merged commit 00a5e66 into rust-lang:master Jul 28, 2015

This was referenced Jul 28, 2015

Assorted diagnostics improvements #27313

Merged

allow constant evaluation of function calls #26848

Merged

oli-obk deleted the move_get_name_get_ident_to_impl branch July 29, 2015 10:38

zsiciarz added a commit to zsiciarz/json_macros that referenced this pull request Jul 29, 2015

Update for recent changes in libsyntax

4c1208a

See rust-lang/rust#27234

zsiciarz mentioned this pull request Jul 29, 2015

Update for recent changes in libsyntax tomjakubowski/json_macros#20

Merged

zsiciarz added a commit to zsiciarz/docopt.rs that referenced this pull request Jul 29, 2015

Update for recent changes in libsyntax

18ffb56

See rust-lang/rust#27234

zsiciarz mentioned this pull request Jul 29, 2015

Update for recent changes in libsyntax docopt/docopt.rs#134

Merged

jashank mentioned this pull request Jul 31, 2015

compile failed due to now-missing as_str() rust-lang/rust-clippy#120

Closed

dgrunwald added a commit to dgrunwald/interpolate_idents that referenced this pull request Aug 2, 2015

Adjust to libsyntax API change (rust-lang/rust#27234)

2215f3b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move `get_ident` and `get_name` free function onto the `Ident` and `Name` types #27234

move `get_ident` and `get_name` free function onto the `Ident` and `Name` types #27234

oli-obk commented Jul 23, 2015

rust-highfive commented Jul 23, 2015

eddyb commented Jul 23, 2015

oli-obk Jul 23, 2015

alexcrichton commented Jul 23, 2015

nikomatsakis commented Jul 23, 2015

bors commented Jul 26, 2015

oli-obk commented Jul 27, 2015

oli-obk Jul 27, 2015

eddyb commented Jul 27, 2015

huonw commented Jul 27, 2015

oli-obk commented Jul 28, 2015

eddyb commented Jul 28, 2015

bors commented Jul 28, 2015

bors commented Jul 28, 2015

bors commented Jul 28, 2015

oli-obk commented Jul 28, 2015

eddyb commented Jul 28, 2015

bors commented Jul 28, 2015

bors commented Jul 28, 2015

bors commented Jul 28, 2015

		@@ -313,10 +313,10 @@ mod svh_visitor {
		// Ad-hoc overloading between Ident and Name to their intern table lookups.
		trait InternKey { fn get_content(self) -> token::InternedString; }

move get_ident and get_name free function onto the Ident and Name types #27234

move get_ident and get_name free function onto the Ident and Name types #27234

Conversation

oli-obk commented Jul 23, 2015

rust-highfive commented Jul 23, 2015

eddyb commented Jul 23, 2015

oli-obk Jul 23, 2015

Choose a reason for hiding this comment

alexcrichton commented Jul 23, 2015

nikomatsakis commented Jul 23, 2015

bors commented Jul 26, 2015

oli-obk commented Jul 27, 2015

oli-obk Jul 27, 2015

Choose a reason for hiding this comment

eddyb commented Jul 27, 2015

huonw commented Jul 27, 2015

oli-obk commented Jul 28, 2015

eddyb commented Jul 28, 2015

bors commented Jul 28, 2015

bors commented Jul 28, 2015

bors commented Jul 28, 2015

oli-obk commented Jul 28, 2015

eddyb commented Jul 28, 2015

bors commented Jul 28, 2015

bors commented Jul 28, 2015

bors commented Jul 28, 2015

move `get_ident` and `get_name` free function onto the `Ident` and `Name` types #27234

move `get_ident` and `get_name` free function onto the `Ident` and `Name` types #27234