-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Re-export more rustc_span::symbol
things from rustc_span
.
#134243
base: master
Are you sure you want to change the base?
Re-export more rustc_span::symbol
things from rustc_span
.
#134243
Conversation
Some changes occurred in cfg and check-cfg configuration cc @Urgau
cc @davidtwco, @compiler-errors, @TaKO8Ki Some changes occurred in need_type_info.rs cc @lcnr HIR ty lowering was modified cc @fmease Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt Some changes occurred to the CTFE machinery cc @rust-lang/wg-const-eval Some changes occurred in match lowering cc @Nadrieril Some changes occurred in check-cfg diagnostics cc @Urgau These commits modify compiler targets. Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri |
588869a
to
5857bd7
Compare
@@ -68,7 +68,7 @@ mod span_encoding; | |||
pub use span_encoding::{DUMMY_SP, Span}; | |||
|
|||
pub mod symbol; | |||
pub use symbol::{Symbol, sym}; | |||
pub use symbol::{Ident, MacroRulesNormalizedIdent, Symbol, kw, sym}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the tedious review. This is the line of note. The rest is just shortening rustc_span::symbol::
qualifiers.
I don't think I find this valuable. Reducing/merging imports is not very useful imo unless it allows removing of crate deps via reexports |
I do think it's valuable. I think it's silly that you can import |
I don't feel too strongly either way since the nesting doesn't seem very steep. Anyway, I'll look at this tmrw since there's several files to go through. |
@nnethercote I'd like to opt out of these changes for rustfmt. |
☔ The latest upstream changes (presumably #133099) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I looked at the changes individually:
- We should drop rustfmt changes as @ytmimi requested.
- We should ask T-rustdoc if they want to opt-in to the rustdoc import changes (or at least in this PR).
- We should ask T-clippy if they want to opt-in to the clippy import changes (or at least in this PR).
- Compiler changes seem reasonable to me. If we want to make the re-exports/imports of {
Ident
,kw
,sym
} by downstream compiler crates and tool users more consistent, I guess better get it done earlier than later.
src/librustdoc/clean/auto_trait.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: @rust-lang/rustdoc would you like to opt-in to the import change (in this PR), or do you want to keep this untouched?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: @rust-lang/clippy do you want to opt-in to this import change (in this PR)? Or do you want to keep this untouched?
The diff from this file sums up the motivation for this change:
It's silly that |
5857bd7
to
a349719
Compare
`rustc_span::symbol` defines some things that are re-exported from `rustc_span`, such as `Symbol` and `sym`. But it doesn't re-export some closely related things such as `Ident` and `kw`. So you can do `use rustc_span::{Symbol, sym}` but you have to do `use rustc_span::symbol::{Ident, kw}`, which is inconsistent for no good reason. This commit re-exports `Ident`, `kw`, and `MacroRulesNormalizedIdent`, and changes many `rustc_span::symbol::` qualifiers to `rustc_span::`. This is a 300+ net line of code reduction, mostly because many files with two `use rustc_span` items can be reduced to one.
a349719
to
4d6ed1d
Compare
rustc_span::symbol
defines some things that are re-exported fromrustc_span
, such asSymbol
andsym
. But it doesn't re-export some closely related things such asIdent
andkw
. So you can douse rustc_span::{Symbol, sym}
but you have to douse rustc_span::symbol::{Ident, kw}
, which is inconsistent for no good reason.This commit re-exports
Ident
,kw
, andMacroRulesNormalizedIdent
, and changes manyrustc_span::symbol::
qualifiers torustc_span::
. This is a 300+ net line of code reduction, mostly because many files with twouse rustc_span
items can be reduced to one.r? @jieyouxu