Clean up `ast::Attribute`, `ast::CrateConfig`, and string interning#37824

jseyfried · 2016-11-17T09:47:11Z

This PR

removes ast::Attribute_ (changing Attribute from Spanned<Attribute_> to a struct),
moves a MetaItem's name from the MetaItemKind variants to a field of MetaItem,
avoids needlessly wrapping ast::MetaItem with P,
moves string interning into syntax::symbol (ast::Name is a reexport of symbol::Symbol for now),
replaces InternedString with Symbol in the AST, HIR, and various other places, and
refactors ast::CrateConfig from a Vec to a HashSet.

r? @eddyb

eddyb

LGTM modulo comments. cc @rust-lang/compiler

eddyb · 2016-11-17T10:22:27Z

src/librustc/hir/map/definitions.rs

I think that these are InternedString so they hash their contents instead of the integer.

Good point, I got overzealous with the InternedString -> Symbol refactoring.

Maybe make a type SymbolStringContent?

eddyb · 2016-11-17T10:26:14Z

src/librustc/ty/sty.rs

Same here, the intention is to sort the strings themselves.

eddyb · 2016-11-17T10:32:20Z

src/libsyntax/symbol.rs

Year is 2016.

petrochenkov · 2016-11-17T14:29:38Z

src/libsyntax/symbol.rs

This is a suspicious line.
InternedString is supposed to be simply a wrapper around Rc<str> now not tied to symbol interner, right? InternedString::new for example just creates a new Rc and intern_and_get_ident was removed by this PR.
At the same time decode not only creates an Rc but also pushes it into the interner.
One of them is probably wrong.

InternedString::new should probably go away.

It depends on what the remaining uses of InternedString are.
Is it used only for hashing/encoding/etc Symbols as strings? In this case something like SymbolStringContent is probably a better solution and InternedString should go away completely, including new.

After this PR, InternedString should be semantically equivalent to SymbolStringContent -- we can rename later. I don't think InternedString is that bad of a name.

I removed InternedString::new.

eddyb · 2016-11-17T14:30:07Z

The one other thing I'd like to see is a symbol!("foo") macro that uses a #[thread_local] static Cell<Symbol> to cache the interning of "foo". It should prove at least marginally faster than string comparisons.
But that experiment can be left to another PR, just wanted to jot it down before I forget.

bors · 2016-11-17T19:02:17Z

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

jseyfried · 2016-11-20T06:22:41Z

@eddyb this should be ready to land now -- the second commit and the last three commits new since last review.

eddyb

r=me with comments addressed.

eddyb · 2016-11-20T10:03:41Z

src/librustc/ty/context.rs

Can't this and crate_name above be Symbol?

Yeah, done.

eddyb · 2016-11-20T10:05:05Z

src/librustc/ty/mod.rs

Seems like original_crate_name should return a Symbol.

eddyb · 2016-11-20T10:08:35Z

src/librustc_driver/lib.rs

Hmm, doesn't this expose HashSet ordering? Even if you were to use a BTreeSet it would expose intern order. Maybe collect strings instead of printing them, to a Vec and just sort that?

eddyb · 2016-11-20T10:12:44Z

src/librustc_metadata/creader.rs

Can't this be written as .to_string() directly?

Yeah, but no longer relevant after using Symbol instead of String in NativeLibrary.

eddyb · 2016-11-20T10:17:27Z

src/librustc_trans/mir/block.rs

Might make sense to change loc.file.name to Symbol - in fact, any String in the compiler that is not on some transient and/or error path, may benefit from it.

Agreed, but best for a future PR.

eddyb · 2016-11-20T10:26:48Z

src/libsyntax/symbol.rs

Maybe add a comment about its thread locality.

eddyb · 2016-11-20T10:30:27Z

src/libsyntax/symbol.rs

The pub field is dangerous, you'll have to privatize it before attempting any optimizations (if the fallout is minimal, maybe do it here).

eddyb · 2016-11-20T10:32:11Z

src/libsyntax/symbol.rs

My comment is no longer relevant (we switched from returning Rc to taking a closure).

eddyb · 2016-11-20T10:33:21Z

src/libsyntax/symbol.rs

This is unsound: you keep references to the string contents around.

I made reset_interner unsafe. It isn't used in rustc itself, but rusti wants to be able to reset thread local state.

Is there a real point to that? The interner would just have a "warm start".

Looks like rusti isn't using driver::reset_thread_local_state at all anymore, so probably not (removed).

eddyb · 2016-11-20T10:37:01Z

src/libsyntax/symbol.rs

Eventual optimization opportunity: store the CodeMap in the interner, guarantee this "symbol string contents" type always points to something owned by the interner, use it in place of &str in the lexer, with appropriate slicing & whatnot, and allow interning it without allocating again - although this requires the arena-based interner too.

eddyb · 2016-11-20T10:44:55Z

cc @rust-lang/compiler

eddyb · 2016-11-21T00:24:12Z

@bors r+

bors · 2016-11-21T00:24:13Z

📌 Commit 5400c4f has been approved by eddyb

bors · 2016-11-21T02:31:47Z

⌛ Testing commit 5400c4f with merge 531bd42...

jseyfried · 2016-11-21T09:08:59Z

@bors r=eddyb

bors · 2016-11-21T09:08:59Z

📌 Commit 60b5372 has been approved by eddyb

bors · 2016-11-21T09:16:49Z

⌛ Testing commit 60b5372 with merge a56532f...

bors · 2016-11-21T10:54:45Z

💔 Test failed - auto-win-msvc-64-opt-rustbuild

jseyfried · 2016-11-21T12:17:29Z

@bors r=eddyb

bors · 2016-11-21T12:17:29Z

📌 Commit a8e86f0 has been approved by eddyb

bors · 2016-11-21T14:08:48Z

⌛ Testing commit a8e86f0 with merge ebec554...

@eddyb

Clean up `ast::Attribute`, `ast::CrateConfig`, and string interning This PR - removes `ast::Attribute_` (changing `Attribute` from `Spanned<Attribute_>` to a struct), - moves a `MetaItem`'s name from the `MetaItemKind` variants to a field of `MetaItem`, - avoids needlessly wrapping `ast::MetaItem` with `P`, - moves string interning into `syntax::symbol` (`ast::Name` is a reexport of `symbol::Symbol` for now), - replaces `InternedString` with `Symbol` in the AST, HIR, and various other places, and - refactors `ast::CrateConfig` from a `Vec` to a `HashSet`. r? @eddyb

bors · 2016-11-21T17:23:02Z

“Clean up `ast::Attribute`, `ast::CrateConfig`, and string interning” Resolves these errors: error[E0425]: unresolved name `token::intern` --> src/lib.rs:27:35 | 27 | reg.register_syntax_extension(token::intern("quickcheck"), | ^^^^^^^^^^^^^ unresolved name error[E0425]: unresolved name `token::str_to_ident` --> src/lib.rs:86:23 | 86 | let check_ident = token::str_to_ident("quickcheck"); | ^^^^^^^^^^^^^^^^^^^ unresolved name error[E0425]: unresolved name `token::intern_and_get_ident` --> src/lib.rs:107:34 | 107 | span, cx.meta_word(span, token::intern_and_get_ident("test")))); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ unresolved name error: aborting due to 3 previous errors Signed-off-by: Anders Kaseorg <andersk@mit.edu>

rust-highfive assigned eddyb Nov 17, 2016

eddyb suggested changes Nov 17, 2016

View reviewed changes

jseyfried force-pushed the symbols branch 2 times, most recently from 520c3c9 to fc519bb Compare November 17, 2016 14:06

petrochenkov reviewed Nov 17, 2016

View reviewed changes

jseyfried force-pushed the symbols branch 7 times, most recently from be6cd9d to fbb1713 Compare November 20, 2016 05:41

eddyb suggested changes Nov 20, 2016

View reviewed changes

jseyfried added 6 commits November 20, 2016 11:46

Refactor away ast::Attribute_.

3ea2bc4

Avoid clearing the string interner.

ff49942

Refactor MetaItemKind to use Names instead of InternedStrings.

a262641

Refactor CrateConfig.

4b9b0d3

Move MetaItemKind's Name to a field of MetaItem.

e97686d

Refactor P<ast::MetaItem> -> ast::MetaItem.

f177a00

jseyfried force-pushed the symbols branch from fbb1713 to 87bf90d Compare November 20, 2016 23:35

Move syntax::util::interner -> syntax::symbol, cleanup.

d2f8fb0

jseyfried force-pushed the symbols branch 2 times, most recently from 6698d2c to 5400c4f Compare November 21, 2016 00:03

jseyfried added 2 commits November 21, 2016 09:00

Remove Rc from the interner.

cc74068

Fix incremental compilation hashing.

c9935e4

jseyfried force-pushed the symbols branch from 8fed18e to 60b5372 Compare November 21, 2016 09:05

Fix fallout in rustdoc and tests.

a8e86f0

jseyfried force-pushed the symbols branch from 60b5372 to a8e86f0 Compare November 21, 2016 12:16

bors mentioned this pull request Nov 21, 2016

Add debug flag -Z print-type-sizes for instrumention type/variant sizes #37770

Merged

bors merged commit a8e86f0 into rust-lang:master Nov 21, 2016

This was referenced Nov 21, 2016

Change HirVec<P<T>> to HirVec<T> in hir:: Expr. #37642

Merged

i128 and u128 support #37900

Closed

parser: simplify directory ownership semantics #37602

Merged

Expand is_uninhabited #36449

Merged

limit the length of types in monomorphization #37789

Merged

jseyfried deleted the symbols branch November 21, 2016 22:05

lambda-fairy mentioned this pull request Nov 23, 2016

Error while compiling serde_codegen on latest nightly serde-rs/serde#627

Closed

This was referenced Nov 23, 2016

Does not compile on 1.15.0-nightly (7b3eeea22 2016-11-21) rust-lang/rust-clippy#1356

Closed

Update to syntex 0.51 serde-deprecated/aster#125

Merged

jseyfried mentioned this pull request Dec 6, 2016

Nightly crashes trying to compile a naked function macro with inline asm #37366

Closed

andersk mentioned this pull request Dec 24, 2016

quickcheck_macros: Update for rust-lang/rust#37824 BurntSushi/quickcheck#153

Merged

Uh oh!

Conversation

jseyfried commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eddyb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb commented Nov 17, 2016

Uh oh!

bors commented Nov 17, 2016

Uh oh!

jseyfried commented Nov 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eddyb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jseyfried commented Nov 17, 2016 •

edited

Loading

petrochenkov Nov 17, 2016 •

edited

Loading

petrochenkov Nov 17, 2016 •

edited

Loading

jseyfried commented Nov 20, 2016 •

edited

Loading