Skip to content

rustc_resolve cleanups - #125105

Merged
bors merged 4 commits into
rust-lang:masterfrom
nnethercote:rustc_resolve-cleanups
May 18, 2024
Merged

rustc_resolve cleanups#125105
bors merged 4 commits into
rust-lang:masterfrom
nnethercote:rustc_resolve-cleanups

Conversation

@nnethercote

Copy link
Copy Markdown
Contributor

Some improvements I found while looking through this code.

r? @estebank

The `#[allow(rustdoc:private_intra_doc_links)]` isn't necessary.

Also sort them, as is done in other files like
`compiler/rustc_errors/src/lib.rs`.
Explicit imports are more standard nowadays and easier to read.
Some minor (English only) heroics are performed to print error messages
like "5th rule of macro `m` is never used". The form "rule rust-lang#5 of macro
`m` is never used" is just as good and much simpler to implement.
@rustbot 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 May 14, 2024
}

/// Convert the given number into the corresponding ordinal
pub(crate) fn ordinalize(v: usize) -> String {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this conflicts with #125042, but I think that one can use just the number too

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In #125042, the function is copied, not referenced (put aside whether it is good or not).

@estebank estebank left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bors r+

use rustc_middle::middle::privacy::{EffectiveVisibilities, EffectiveVisibility};
use rustc_middle::ty::Visibility;
use std::mem;
use tracing::info;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are all of these additions from tracing in different files needed?

@nnethercote nnethercote May 18, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. The #[macro_use] extern crate tracing; gave us implicit import of all tracing macros. With that gone, we must do explicit imports, and tracing macros are used in lots of places.

--> $DIR/unused-macro-rules.rs:8:5
|
LL | (two) => { 2 };
| ^^^^^

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Future note: we should emit a single error for all of the unused rules in a single macro.

@estebank

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented May 18, 2024

Copy link
Copy Markdown
Collaborator

📌 Commit 5134a04 has been approved by estebank

It is now in the queue for this repository.

@bors 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 May 18, 2024
@bors

bors commented May 18, 2024

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 5134a04 with merge 36c0a6d...

@bors

bors commented May 18, 2024

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 36c0a6d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 18, 2024
@bors
bors merged commit 36c0a6d into rust-lang:master May 18, 2024
@rustbot rustbot added this to the 1.80.0 milestone May 18, 2024
@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (36c0a6d): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 1.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.9% [2.9%, 2.9%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.0% [-1.0%, -1.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.0% [-1.0%, 2.9%] 2

Cycles

Results (primary 1.6%, secondary 1.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
4.4% [4.4%, 4.4%] 1
Regressions ❌
(secondary)
4.8% [4.8%, 4.8%] 1
Improvements ✅
(primary)
-1.3% [-1.3%, -1.3%] 1
Improvements ✅
(secondary)
-2.6% [-2.6%, -2.6%] 1
All ❌✅ (primary) 1.6% [-1.3%, 4.4%] 2

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 668.851s -> 669.26s (0.06%)
Artifact size: 316.06 MiB -> 316.04 MiB (-0.01%)

@nnethercote
nnethercote deleted the rustc_resolve-cleanups branch May 20, 2024 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-by-bors This PR was explicitly merged by bors. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants