Skip to content

[arithmetic_side_effects] Cache symbols #10675

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

Merged
merged 1 commit into from
Apr 20, 2023
Merged

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Apr 20, 2023

An internal-only modification to speed up the processing of symbols because "intern" isn't very cheap, even more when you are doing the same thing for every method expression.

changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Apr 20, 2023

r? @llogiq

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 20, 2023
@llogiq
Copy link
Contributor

llogiq commented Apr 20, 2023

Good catch!

@bors r+

@bors
Copy link
Contributor

bors commented Apr 20, 2023

📌 Commit 0b16f80 has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Apr 20, 2023

⌛ Testing commit 0b16f80 with merge bc9892d...

bors added a commit that referenced this pull request Apr 20, 2023
[arithmetic_side_effects] Cache symbols

An internal-only modification to speed up the processing of symbols because "intern" isn't very cheap, even more when you are doing the same thing for every method expression.
@bors
Copy link
Contributor

bors commented Apr 20, 2023

💔 Test failed - checks-action_test

@llogiq
Copy link
Contributor

llogiq commented Apr 20, 2023

Ah, we missed the changelog line. @bors retry

@bors
Copy link
Contributor

bors commented Apr 20, 2023

⌛ Testing commit 0b16f80 with merge c976ad0...

@bors
Copy link
Contributor

bors commented Apr 20, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing c976ad0 to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Apr 20, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing c976ad0 to master...

@bors bors merged commit c976ad0 into rust-lang:master Apr 20, 2023
@klensy
Copy link
Contributor

klensy commented Apr 21, 2023

Isn't it simpler to add that strings to symbol table, and then just filter on symbols, skipping str<->String comparison/conversions?

@c410-f3r
Copy link
Contributor Author

That is what I asked at #10615 (comment)

IIRC, it is not rare to see PRs in rust-last/rust trimming pre-registered symbols and Clippy already has a bunch of interns, therefore, caching seemed to be the natural choice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants