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

Rename `LocalInternedString` and more #65776

Merged

Conversation

@nnethercote
Copy link
Contributor

nnethercote commented Oct 24, 2019

This PR renames LocalInternedString as SymbolStr, removes an unnecessary impl from it, improves comments, and cleans up some SymbolStr uses.

r? @estebank

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Oct 24, 2019

I mentioned elsewhere my suggestion of SymbolContents (another one could be SymbolData), and I feel like SymbolStr isn't clear enough of a name.
But it's really just a bikeshed, and not that important.

if &*name == "rust_eh_personality" ||
&*name == "rust_eh_register_frames" ||
&*name == "rust_eh_unregister_frames" {
if name == "rust_eh_personality" ||

This comment has been minimized.

Copy link
@Centril

Centril Oct 24, 2019

Member

Should these be sym::s?

This comment has been minimized.

Copy link
@eddyb

eddyb Oct 24, 2019

Member

We can probably replace most string literals with valid literals in them, in the compiler, with sym::s.

I'd try this if I was in front of my laptop:

rg '"[_a-zA-Z][_a-zA-Z0-9]*"' src/lib*

This comment has been minimized.

Copy link
@nnethercote

nnethercote Oct 24, 2019

Author Contributor

rust_eh_personality already is a symbol. We could add the other two. For all potential static symbols there is a decision about whether the identifier is hot enough or frequently used enough for it to be worthwhile, and that decision is not always clear-cut.

"Intern all the static strings" is a possibility, but it would add a lot more due to functions like this one:

let n = &*name.as_str();
let human_readable = match (n, value) {
("unix", None) => "Unix",
("windows", None) => "Windows",
("debug_assertions", None) => "debug-assertions enabled",
("target_os", Some(os)) => match &*os.as_str() {
"android" => "Android",
"dragonfly" => "DragonFly BSD",
"emscripten" => "Emscripten",
"freebsd" => "FreeBSD",
"fuchsia" => "Fuchsia",
"haiku" => "Haiku",
"ios" => "iOS",
"l4re" => "L4Re",
"linux" => "Linux",
"macos" => "macOS",
"netbsd" => "NetBSD",
"openbsd" => "OpenBSD",
"redox" => "Redox",
"solaris" => "Solaris",
"windows" => "Windows",
_ => "",
},
("target_arch", Some(arch)) => match &*arch.as_str() {
"aarch64" => "AArch64",
"arm" => "ARM",
"asmjs" => "JavaScript",
"mips" => "MIPS",
"mips64" => "MIPS-64",
"msp430" => "MSP430",
"powerpc" => "PowerPC",
"powerpc64" => "PowerPC-64",
"s390x" => "s390x",
"sparc64" => "SPARC64",
"wasm32" => "WebAssembly",
"x86" => "x86",
"x86_64" => "x86-64",
_ => "",
},
("target_vendor", Some(vendor)) => match &*vendor.as_str() {
"apple" => "Apple",
"pc" => "PC",
"rumprun" => "Rumprun",
"sun" => "Sun",
"fortanix" => "Fortanix",
_ => ""
},
("target_env", Some(env)) => match &*env.as_str() {
"gnu" => "GNU",
"msvc" => "MSVC",
"musl" => "musl",
"newlib" => "Newlib",
"uclibc" => "uClibc",
"sgx" => "SGX",
_ => "",
},
("target_endian", Some(endian)) => return write!(fmt, "{}-endian", endian),
("target_pointer_width", Some(bits)) => return write!(fmt, "{}-bit", bits),
("target_feature", Some(feat)) =>
if self.1 {
return write!(fmt, "<code>{}</code>", feat);
} else {
return write!(fmt, "target feature <code>{}</code>", feat);
},
_ => "",
};

This comment has been minimized.

Copy link
@nnethercote

nnethercote Oct 24, 2019

Author Contributor

(I should add: I have already converted all the static identifiers that I though were worth converting. Other people may reasonably think that more should be converted.)

/// operation because it requires locking the symbol interner.
pub fn as_str(self) -> LocalInternedString {
/// Convert the name to a `SymbolStr`. This is a slowish operation because
/// it requires locking the symbol interner.

This comment has been minimized.

Copy link
@eddyb

eddyb Oct 24, 2019

Member

cc @Manishearth could elsa have a FrozenIndexSet that is also thread-safe without a lock?
In this case, just a Sync FrozenVec might be enough.

@petrochenkov petrochenkov removed their assignment Oct 25, 2019
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Oct 27, 2019

What's the status here? It's not clear to me if @eddyb or @Centril is the reviewer. There are some comments but none of them seem like show-stoppers.

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Oct 27, 2019

Can I please not be the reviewer? =D

It appears @estebank is. :)

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Oct 27, 2019

Indeed, I mixed up GitHub's "assignees" and "reviewers" fields. @estebank is the reviewer.

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Oct 31, 2019

@bors r+ rollup=never

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 31, 2019

📌 Commit 7a466f0 has been approved by estebank

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 1, 2019

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

nnethercote added 7 commits Oct 21, 2019
It makes the relationship with `Symbol` clearer. The `Str` suffix
matches the existing `Symbol::as_str()` method nicely, and is also
consistent with it being a wrapper of `&str`.
Including removing a bunch of unnecessary `.as_str()` calls, and a bunch
of unnecessary sigils.
Because it's highly magical, which goes against the goal of keeping
`SymbolStr` simple. Plus it's only used in a handful of places that
only require minor changes.
@nnethercote nnethercote force-pushed the nnethercote:rename-LocalInternedString-and-more branch from 7a466f0 to d0db290 Nov 1, 2019
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Nov 1, 2019

I rebased.

@bors r=estebank

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 1, 2019

📌 Commit d0db290 has been approved by estebank

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 3, 2019

⌛️ Testing commit d0db290 with merge 0afc234...

bors added a commit that referenced this pull request Nov 3, 2019
…e, r=estebank

Rename `LocalInternedString` and more

This PR renames `LocalInternedString` as `SymbolStr`, removes an unnecessary `impl` from it, improves comments, and cleans up some `SymbolStr` uses.

r? @estebank
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 3, 2019

The job x86_64-gnu-tools of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-11-03T07:10:04.2092474Z  Caused By: "https://rust-lang.github.io/chalk/doc/chalk_parse/index.html" returned 404 Not Found
2019-11-03T07:10:04.2092993Z  Caused By: "https://rust-lang.github.io/chalk/doc/chalk/index.html" returned 404 Not Found
2019-11-03T07:10:04.2093349Z  Caused By: "https://rust-lang.github.io/chalk/doc/chalki/index.html" returned 404 Not Found
2019-11-03T07:10:04.2093753Z  Caused By: "https://doc.rust-lang.org/nightly/nightly-rustc/syntax/parse/struct.ParseSess.html" returned 404 Not Found
2019-11-03T07:10:04.2093937Z  Caused By: There was an error while fetching "http://www.cs.rice.edu/%7Eyguo/pubs/PID824943.pdf", http://www.cs.rice.edu/%7Eyguo/pubs/PID824943.pdf: timed out
2019-11-03T07:10:04.2094114Z 
2019-11-03T07:10:04.2097005Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/rustbook" "linkcheck" "/checkout/src/doc/rustc-guide"
2019-11-03T07:10:04.2098340Z expected success, got: exit code: 101
2019-11-03T07:10:04.2098741Z 
---
2019-11-03T07:16:38.3107307Z 
2019-11-03T07:16:43.1579761Z error[E0277]: the trait bound `syntax_pos::symbol::SymbolStr: std::convert::AsRef<std::ffi::OsStr>` is not satisfied
2019-11-03T07:16:43.1581114Z     --> src/tools/clippy/clippy_lints/src/path_buf_push_overwrite.rs:53:44
2019-11-03T07:16:43.1581796Z      |
2019-11-03T07:16:43.1582827Z 53   |             if let pushed_path = Path::new(&path_lit.as_str());
2019-11-03T07:16:43.1583837Z      |                                            ^^^^^^^^^^^^^^^^^^ the trait `std::convert::AsRef<std::ffi::OsStr>` is not implemented for `syntax_pos::symbol::SymbolStr`
2019-11-03T07:16:43.1585151Z     ::: /checkout/src/libstd/path.rs:1814:19
2019-11-03T07:16:43.1585719Z      |
2019-11-03T07:16:43.1585719Z      |
2019-11-03T07:16:43.1586396Z 1814 |     pub fn new<S: AsRef<OsStr> + ?Sized>(s: &S) -> &Path {
2019-11-03T07:16:43.1587769Z 
2019-11-03T07:16:44.0585621Z error: aborting due to 5 previous errors
2019-11-03T07:16:44.0585803Z 
2019-11-03T07:16:44.0592176Z Some errors have detailed explanations: E0277, E0432.
---
2019-11-03T07:55:01.6353394Z   local time: Sun Nov  3 07:55:01 UTC 2019
2019-11-03T07:55:01.9051349Z   network time: Sun, 03 Nov 2019 07:55:01 GMT
2019-11-03T07:55:01.9055246Z == end clock drift check ==
2019-11-03T07:55:02.9035817Z 
2019-11-03T07:55:02.9162598Z ##[error]Bash exited with code '1'.
2019-11-03T07:55:02.9203954Z ##[section]Starting: Checkout
2019-11-03T07:55:02.9206104Z ==============================================================================
2019-11-03T07:55:02.9206207Z Task         : Get sources
2019-11-03T07:55:02.9206319Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 3, 2019

💔 Test failed - checks-azure

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Nov 5, 2019

No Tools Breakage Week is over, or close enough that by the time this gets through the queue it will be.

@bors r=estebank

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 5, 2019

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
  • There's another pull request that is currently being tested, blocking this pull request: #66121
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 5, 2019

📌 Commit d0db290 has been approved by estebank

Centril added a commit to Centril/rust that referenced this pull request Nov 6, 2019
…ing-and-more, r=estebank

Rename `LocalInternedString` and more

This PR renames `LocalInternedString` as `SymbolStr`, removes an unnecessary `impl` from it, improves comments, and cleans up some `SymbolStr` uses.

r? @estebank
bors added a commit that referenced this pull request Nov 6, 2019
Rollup of 9 pull requests

Successful merges:

 - #65776 (Rename `LocalInternedString` and more)
 - #65973 (caller_location: point to macro invocation sites, like file!/line!, and use in core::panic!.)
 - #66015 (librustc_lexer: Refactor the module)
 - #66062 (Configure LLVM module PIC level)
 - #66086 (bump smallvec to 1.0)
 - #66092 (Use KERN_ARND syscall for random numbers on NetBSD, same as FreeBSD.)
 - #66103 (Add target thumbv7neon-unknown-linux-musleabihf)
 - #66133 (Update the bundled `wasi-libc` repository)
 - #66139 (use American spelling for `pluralize!`)

Failed merges:

r? @ghost
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Nov 6, 2019

@bors rollup=maybe

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 6, 2019

⌛️ Testing commit d0db290 with merge e8b190a...

@bors bors merged commit d0db290 into rust-lang:master Nov 6, 2019
4 of 5 checks passed
4 of 5 checks passed
homu Testing commit d0db29003975d8c4b3a552ff8c3a68435173cdc7 with merge e8b190ac4ad79e58d21ee1d573529ff74d8eedaa...
Details
pr Build #20191101.27 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
lzutao added a commit to lzutao/rust-clippy that referenced this pull request Nov 6, 2019
lzutao added a commit to lzutao/rust-clippy that referenced this pull request Nov 6, 2019
bors added a commit to rust-lang/rust-clippy that referenced this pull request Nov 6, 2019
 rustup rust-lang/rust#65776

changelog: none
@nnethercote nnethercote deleted the nnethercote:rename-LocalInternedString-and-more branch Nov 6, 2019
calebcartwright added a commit to calebcartwright/rustfmt that referenced this pull request Jan 16, 2020
AFAICT these changes are required based on the changes `LocalInternedString` and `SymbolStr` in
rust-lang/rust#65776
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.