A drive-by rewrite of give_region_a_name() by amandasystems · Pull Request #120704 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation9 Commits2 Checks0 Files changed

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

This drive-by rewrite makes the cache-updating nature of the method clearer, using the Entry API into the hash table for region names to capture the update-insert nature of the method. May be marginally more efficient since it only runtime-borrows and indexes the map once, but in this context the performance impact is almost certainly completely negligible.

Note that this commit should preserve all externally visible behaviour. Notably, it preserves the debug logging:

1. printing even in the case of a None for the new computed name, and
2. only printing on new values, begin silent on reused values

This rewrite makes the cache-updating nature of the function slightly clearer, using the Entry API into the hash table for region names to capture the update-insert nature of the method. May be marginally more efficient since it only runtime-borrows the map once, but in this context the performance impact is almost certainly completely negligible.

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wesleywiser (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make RegionNameSource by using Symbol::intern to intern a Symbol rather than a String for the only !Copy variant in that enum: AnonRegionFromYieldTy? In that case, then you don't need to be using references + cloning here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll look into it! See I knew making a stupid tiny commit would be a good idea to learn something I hadn’t thought about!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please give this final commit a useful commit title? then i can approve

rustbot added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Feb 7, 2024

Of course, sorry, I thought the commits would get squashed anyway.

@rustbot review

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Feb 8, 2024

📌 Commit 795be51 has been approved by compiler-errors

It is now in the queue for this repository.

bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Feb 8, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Feb 8, 2024

…write, r=compiler-errors

A drive-by rewrite of give_region_a_name()

This drive-by rewrite makes the cache-updating nature of the method clearer, using the Entry API into the hash table for region names to capture the update-insert nature of the method. May be marginally more efficient since it only runtime-borrows and indexes the map once, but in this context the performance impact is almost certainly completely negligible.

Note that this commit should preserve all externally visible behaviour. Notably, it preserves the debug logging:

1. printing even in the case of a None for the new computed name, and
2. only printing on new values, begin silent on reused values

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Feb 8, 2024

…write, r=compiler-errors

A drive-by rewrite of give_region_a_name()

This drive-by rewrite makes the cache-updating nature of the method clearer, using the Entry API into the hash table for region names to capture the update-insert nature of the method. May be marginally more efficient since it only runtime-borrows and indexes the map once, but in this context the performance impact is almost certainly completely negligible.

Note that this commit should preserve all externally visible behaviour. Notably, it preserves the debug logging:

1. printing even in the case of a None for the new computed name, and
2. only printing on new values, begin silent on reused values

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Feb 8, 2024

…write, r=compiler-errors

A drive-by rewrite of give_region_a_name()

This drive-by rewrite makes the cache-updating nature of the method clearer, using the Entry API into the hash table for region names to capture the update-insert nature of the method. May be marginally more efficient since it only runtime-borrows and indexes the map once, but in this context the performance impact is almost certainly completely negligible.

Note that this commit should preserve all externally visible behaviour. Notably, it preserves the debug logging:

1. printing even in the case of a None for the new computed name, and
2. only printing on new values, begin silent on reused values

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Feb 8, 2024

…write, r=compiler-errors

A drive-by rewrite of give_region_a_name()

This drive-by rewrite makes the cache-updating nature of the method clearer, using the Entry API into the hash table for region names to capture the update-insert nature of the method. May be marginally more efficient since it only runtime-borrows and indexes the map once, but in this context the performance impact is almost certainly completely negligible.

Note that this commit should preserve all externally visible behaviour. Notably, it preserves the debug logging:

1. printing even in the case of a None for the new computed name, and
2. only printing on new values, begin silent on reused values

bors added a commit to rust-lang-ci/rust that referenced this pull request

Feb 8, 2024

…iaskrgr

Rollup of 10 pull requests

Successful merges:

• rust-lang#113026 (Introduce run-make V2 infrastructure, a run_make_support library and port over 2 tests as example)
• rust-lang#120589 (std::thread::available_parallelism merging linux/android/freebsd version)
• rust-lang#120590 (Remove unused args from functions)
• rust-lang#120596 ([rustdoc] Correctly generate path for non-local items in source code pages)
• rust-lang#120693 (Invert diagnostic lints.)
• rust-lang#120704 (A drive-by rewrite of give_region_a_name())
• rust-lang#120750 (No need to take ImplTraitContext by ref)
• rust-lang#120765 (Reorder diagnostics API)
• rust-lang#120772 (Remove myself from review rotation.)
• rust-lang#120783 (Add release note for new ambiguous_wide_pointer_comparisons lint)

Failed merges:

• rust-lang#120782 (Fix mir pass ICE in the presence of other errors)

r? @ghost @rustbot modify labels: rollup

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Feb 9, 2024

…write, r=compiler-errors

A drive-by rewrite of give_region_a_name()

This drive-by rewrite makes the cache-updating nature of the method clearer, using the Entry API into the hash table for region names to capture the update-insert nature of the method. May be marginally more efficient since it only runtime-borrows and indexes the map once, but in this context the performance impact is almost certainly completely negligible.

Note that this commit should preserve all externally visible behaviour. Notably, it preserves the debug logging:

1. printing even in the case of a None for the new computed name, and
2. only printing on new values, begin silent on reused values

bors added a commit to rust-lang-ci/rust that referenced this pull request

Feb 9, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Feb 9, 2024

…write, r=compiler-errors

A drive-by rewrite of give_region_a_name()

This drive-by rewrite makes the cache-updating nature of the method clearer, using the Entry API into the hash table for region names to capture the update-insert nature of the method. May be marginally more efficient since it only runtime-borrows and indexes the map once, but in this context the performance impact is almost certainly completely negligible.

Note that this commit should preserve all externally visible behaviour. Notably, it preserves the debug logging:

1. printing even in the case of a None for the new computed name, and
2. only printing on new values, begin silent on reused values

bors added a commit to rust-lang-ci/rust that referenced this pull request

Feb 9, 2024

…iaskrgr

Rollup of 7 pull requests

Successful merges:

• rust-lang#120308 (core/time: avoid divisions in Duration::new)
• rust-lang#120596 ([rustdoc] Correctly generate path for non-local items in source code pages)
• rust-lang#120693 (Invert diagnostic lints.)
• rust-lang#120704 (A drive-by rewrite of give_region_a_name())
• rust-lang#120809 (Use transmute_unchecked in NonZero::new.)
• rust-lang#120817 (Fix more ty::Error ICEs in MIR passes)
• rust-lang#120828 (Fix ErrorGuaranteed unsoundness with stash/steal.)

r? @ghost @rustbot modify labels: rollup

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Feb 9, 2024

…write, r=compiler-errors

A drive-by rewrite of give_region_a_name()

This drive-by rewrite makes the cache-updating nature of the method clearer, using the Entry API into the hash table for region names to capture the update-insert nature of the method. May be marginally more efficient since it only runtime-borrows and indexes the map once, but in this context the performance impact is almost certainly completely negligible.

Note that this commit should preserve all externally visible behaviour. Notably, it preserves the debug logging:

1. printing even in the case of a None for the new computed name, and
2. only printing on new values, begin silent on reused values

bors added a commit to rust-lang-ci/rust that referenced this pull request

Feb 9, 2024

…iaskrgr

Rollup of 9 pull requests

Successful merges:

• rust-lang#113026 (Introduce run-make V2 infrastructure, a run_make_support library and port over 2 tests as example)
• rust-lang#113671 (Make privacy visitor use types more (instead of HIR))
• rust-lang#120308 (core/time: avoid divisions in Duration::new)
• rust-lang#120693 (Invert diagnostic lints.)
• rust-lang#120704 (A drive-by rewrite of give_region_a_name())
• rust-lang#120809 (Use transmute_unchecked in NonZero::new.)
• rust-lang#120817 (Fix more ty::Error ICEs in MIR passes)
• rust-lang#120828 (Fix ErrorGuaranteed unsoundness with stash/steal.)
• rust-lang#120831 (Startup objects disappearing from sysroot)

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Feb 9, 2024

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Feb 9, 2024

Rollup merge of rust-lang#120704 - amandasystems:silly-region-name-rewrite, r=compiler-errors

A drive-by rewrite of give_region_a_name()

This drive-by rewrite makes the cache-updating nature of the method clearer, using the Entry API into the hash table for region names to capture the update-insert nature of the method. May be marginally more efficient since it only runtime-borrows and indexes the map once, but in this context the performance impact is almost certainly completely negligible.

Note that this commit should preserve all externally visible behaviour. Notably, it preserves the debug logging:

1. printing even in the case of a None for the new computed name, and
2. only printing on new values, begin silent on reused values

flip1995 pushed a commit to flip1995/rust that referenced this pull request

Feb 26, 2024

bjorn3 pushed a commit to bjorn3/rust that referenced this pull request

Mar 8, 2024

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.