Start cleaning up the string interner #34772
Conversation
| }).chain(reader::tagged_docs(item, tag_item_unnamed_field).map(|_| { | ||
| let name = intr.intern(&index.to_string()); | ||
| let name = token::with_ident_interner(|interner| interner.intern(index.to_string())); |
There was a problem hiding this comment.
Is calling with_ident_interner even necessary here?
There was a problem hiding this comment.
No, this is equivalent to token::intern(&index.to_string()) (except that it avoids a clone).
There was a problem hiding this comment.
Maybe token::intern should be as generic as Interner::intern?
Eventually this could be a generic Symbol::from, I guess.
I would like it if with_ident_interner was actually private.
There was a problem hiding this comment.
Yeah, I'd like token::intern to be generic but it's a syntax-[breaking-change] since token::intern(&my_string) where my_string: String wouldn't typecheck (intern(my_string) or intern(&*my_string) would, though).
I was planning on making token::intern generic in a future PR with other interner-related syntax-[breaking-change]s.
I agree that with_ident_interner should eventually be private and that token::intern should eventually be Symbol::from (or Symbol::intern to contrast with Symbol::gensym).
|
r=me if none of the interested parties disagree on the changes. |
|
lgtm |
|
@bors r+ |
|
📌 Commit 060b5c5 has been approved by |
|
@jseyfried double points if you also modify the interner to support marking some things as "unclearable" such that clearing the interner preserves them (which will make the clearing we do in the driver more robust and fix a few of the problems with transitioning attributes to tokenstreams) |
|
@cgswords We could also use two interners and only clear one of them. We could make this type-safe by making |
Start cleaning up the string interner r? @eddyb
That's what the compiler is currently doing, but that breaks down when you need, eg, token literals to stick around in a tokenstream (because literals store
Splitting the type out is going to be annoying for the use case I described above---we still want |
|
@cgswords Why do we need some Afaik, we don't need any interned strings ( |
| @@ -477,17 +477,20 @@ pub type IdentInterner = Interner; | |||
| // if an interner exists in TLS, return it. Otherwise, prepare a | |||
| // fresh one. | |||
| // FIXME(eddyb) #8726 This should probably use a thread-local reference. | |||
r? @eddyb