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

Simplify utils::match_def_path, removing a FIXME #4508

Merged
merged 1 commit into from
Sep 6, 2019
Merged

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Sep 6, 2019

changelog: none

This removes the Vec<Symbol> allocation. We still need to call cx.get_def_path, but this should already have been interned, and I don't see how we can keep ergonomics of that function without allocating a Vec.

r? @phansch

@oli-obk
Copy link
Contributor

oli-obk commented Sep 6, 2019

We can actually start getting rid of these string paths. The compiler now has support for diagnostic items. Look at the Debug trait to see how to change libcore to have more diag items

@phansch
Copy link
Member

phansch commented Sep 6, 2019

See Oli's comment :)

cc rust-lang/rust#60966

@llogiq
Copy link
Contributor Author

llogiq commented Sep 6, 2019

I see how declaring diagnostic items will help, but that refactoring will be a larger one. This PR can still be useful in the meantime.

@phansch
Copy link
Member

phansch commented Sep 6, 2019

Oh, I thought it could have been a more granular change. Let's get this merged for now

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 6, 2019

📌 Commit 72058a6 has been approved by phansch

@bors
Copy link
Collaborator

bors commented Sep 6, 2019

⌛ Testing commit 72058a6 with merge f30bf69...

bors added a commit that referenced this pull request Sep 6, 2019
Simplify `utils::match_def_path`, removing a FIXME

changelog: none

This removes the `Vec<Symbol>` allocation. We still need to call `cx.get_def_path`, but this should already have been interned, and I don't see how we can keep ergonomics of that function without allocating a `Vec`.

r? @phansch
@llogiq
Copy link
Contributor Author

llogiq commented Sep 6, 2019

@oli-obk: So do we go through utils/paths.rs, find all the types, methods, etc. and set up Rust PRs with the rustc_diagnostic_item annotations? Also on the Rust side I see a lot of entries in sym, but only one rustc_diagnostic_item annotation on Debug. How do we ensure there's no name clash?

@bors
Copy link
Collaborator

bors commented Sep 6, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: phansch
Pushing f30bf69 to master...

@bors bors merged commit 72058a6 into master Sep 6, 2019
@llogiq llogiq deleted the reduced-symbolism branch September 6, 2019 16:15
@oli-obk
Copy link
Contributor

oli-obk commented Sep 6, 2019

The entries in sym are entirely independent of the names we can use for diagnostic items. If there's already an entry in sym, don't add a new one. Just invent any name you think fits for the item. If it doesn't exist in sym yet, that's fine, either add it or let clippy generate the symbol from a string. I think we could go with not adding anything to the sym list for now

@llogiq
Copy link
Contributor Author

llogiq commented Sep 7, 2019

OK, how do I find out what an item in sym points to?

@oli-obk
Copy link
Contributor

oli-obk commented Sep 7, 2019

You don't. The sym list has nothing to do with items. It's just interned names. Ignore the sym list. It's just a perf improvement, not a necessity

@llogiq
Copy link
Contributor Author

llogiq commented Sep 7, 2019

I see. I tried this for Vec (will push WIP soon), but the tests failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants