Use tcx.symbol_name when determining clashing extern declarations. #80009
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
nagisa
reviewed
Dec 13, 2020
nagisa
reviewed
Dec 13, 2020
| @@ -2595,7 +2596,7 @@ declare_lint! { | |||
| } | |||
|
|
|||
| pub struct ClashingExternDeclarations { | |||
| seen_decls: FxHashMap<Symbol, HirId>, | |||
| seen_decls: FxHashMap<String, HirId>, | |||
There was a problem hiding this comment.
In theory this could just store &'tcx str without an issue, but the impl_lint_pass! macro does not support lints parametric over a lifetime T_T.
There was a problem hiding this comment.
could just store &'tcx str without an issue
Yeah, you're right -- that would let me avoid the allocation completely.
the impl_lint_pass! macro does not support lints parametric over a lifetime T_T
I don't think I'll address this in this PR -- I've just added a FIXME note for now, until impl_lint_pass! can take lifetimes.
|
Seems reasonable to me! |
jumbatm
force-pushed
the
issue79581-overeager-clashing-extern-decl
branch
from
a154be7
to
fa02a8e
Compare
|
This should be good for re-review once green. |
|
@bors: r+ Thanks! |
|
📌 Commit fa02a8ed6405d6cd2e4f5bd27cde9f0c2f59184b has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit fa02a8ed6405d6cd2e4f5bd27cde9f0c2f59184b with merge a0f9320b9d4490efe6beace771710a58f1a3e44c... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
jumbatm
force-pushed
the
issue79581-overeager-clashing-extern-decl
branch
from
fa02a8e
to
4b740ac
Compare
|
Not sure why it failed the first time. @alexcrichton -- can you re-approve? |
|
@bors: retry |
bors
added
S-waiting-on-bors
|
Ah, @bors r=alexcrichton - the prior approval wouldn't have worked I expect since commits have been pushed since then. |
|
📌 Commit 4b740ac has been approved by |
|
☀️ Test successful - checks-actions |
|
@jumbatm @nagisa This was the cause of a minor perf regression. I wonder how much of it is the allocation of the symbol name string vs the call |
|
Thanks for the heads-up. I'll have a look into this. |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 30, 2021
…nagisa clashing_extern_declarations: Use symbol interning to avoid string alloc. Use symbol interning as a hack to avoid allocating a string for every symbol name we store in the seen set. This hopefully addresses the minor perf regression described in rust-lang#80009 (comment). r? `@nagisa`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #79581.
r? @alexcrichton