Conversation
rustbot
added
the
T-rustdoc
rust-highfive
added
the
S-waiting-on-review
CraftSpider
force-pushed
the
rustdoc-ty-fixes
branch
from
1237312 to
3925451
Compare
This comment has been minimized.
This comment has been minimized.
CraftSpider
changed the title
|
Can you squash the "Format" commit into the previous commit? |
|
I reviewed the first few commits. I'll try to review the rest soon. |
camelid
added
the
C-cleanup
CraftSpider
force-pushed
the
rustdoc-ty-fixes
branch
from
3925451 to
2d2163b
Compare
|
Format squashed in |
camelid
left a comment
camelid
left a comment
There was a problem hiding this comment.
This looks good to me overall but I'm a bit unclear about d90138b. Could you explain the goal of that commit? Also, the last commit doesn't actually make hir and ty use the same code paths so the commit message is a little confusing. Most of the code is actually still different. It'd probably be hard to fully merge them until type aliases are represented in ty though.
| sig: ty::PolyFnSig<'_>, | ||
| ) -> FnDecl { | ||
| let mut names = if did.is_local() { &[] } else { cx.tcx.fn_arg_names(did) }.iter(); | ||
| let mut names = did.map_or(&[] as &[_], |did| cx.tcx.fn_arg_names(did)).iter(); |
There was a problem hiding this comment.
Hmm, could this be a behavior change when this is called with an external function? I think if it is, it'd be a bugfix, but I wanted to check. In the inline code path (inline.rs), this is called with Some(did), where did should be non-local.
There was a problem hiding this comment.
This should only change code on local functions, external functions always took the else case and are thus equivalent to Some. Local function behavior should be affected as an intentional fix related to this PR.
There was a problem hiding this comment.
You're right, I got confused. Thanks!
|
Sorry for taking so long to review this by the way! |
camelid
added
S-waiting-on-author
|
Commit d90138b is reverting a change I made in an earlier PR that was necessary before these fixes - There's a JSON test that ensures removing it here didn't cause a regression. The comment about 'same code path' is meant to mean that the two different functions calculate what items are impl items the same way, not that they're fully the same code paths but that they should now use the same rules for what is an 'impl of a trait'. |
CraftSpider
added
S-waiting-on-review
|
@bors r+ |
|
📌 Commit 2d2163b has been approved by |
bors
added
S-waiting-on-bors
Rollup of 7 pull requests Successful merges: - rust-lang#91545 (Generalize "remove `&`" and "add `*`" suggestions to more than one deref) - rust-lang#93385 (Rustdoc ty consistency fixes) - rust-lang#93926 (Lint against more useless `#[must_use]` attributes) - rust-lang#94094 (use BOOL for TCP_NODELAY setsockopt value on Windows) - rust-lang#94384 (Add Atomic*::from_mut_slice) - rust-lang#94448 (5 - Make more use of `let_chains`) - rust-lang#94452 (Sync portable-simd for bitmasks &c.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
6 participants
Changes to make rustdoc cleaning of ty more consistent with hir, and hopefully use it in more places.
r? @camelid