rustdoc: show trait implementors in sidebar #91277
Conversation
rust-highfive
added
the
S-waiting-on-review
| @@ -73,7 +75,7 @@ crate struct Context<'tcx> { | |||
|
|
|||
| // `Context` is cloned a lot, so we don't want the size to grow unexpectedly. | |||
| #[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))] | |||
| rustc_data_structures::static_assert_size!(Context<'_>, 144); | |||
| rustc_data_structures::static_assert_size!(Context<'_>, 184); | |||
There was a problem hiding this comment.
This increase seems to be fine, as when cloned, we create a new map, so perf is neutral (#91257)
|
|
||
| // @has foo/trait.NotImpled.html | ||
| // @!has - '//*[@class="sidebar-title"]/a[@href="#implementors"]' | ||
| // FIXME: Is this the semantics we want |
There was a problem hiding this comment.
Specificly the page is documented like
Because we only write the "Implementations on Foreign Types" header when they are present (
rust/src/librustdoc/html/render/print_item.rs
Line 755 in ccce985
rust/src/librustdoc/html/render/print_item.rs
Lines 782 to 790 in ccce985
If theirs a reason for this, its fine to keep, otherwize I'll change it to be consistant
There was a problem hiding this comment.
It's because the implementors section also list implementations on foreign types (as you can see here for example).
aDotInTheVoid
force-pushed
the
rustdoc-trait-impl-implementors-sidebar
branch
from
1bd2478
to
9f47199
Compare
This comment has been minimized.
This comment has been minimized.
aDotInTheVoid
force-pushed
the
rustdoc-trait-impl-implementors-sidebar
branch
from
9f47199
to
3ff81db
Compare
jyn514
changed the title
|
@GuillaumeGomez do you mind taking over the review? not sure I have time for this |
| @@ -40,4 +40,8 @@ impl Impl { | |||
| crate fn trait_did(&self) -> Option<DefId> { | |||
| self.inner_impl().trait_.as_ref().map(|t| t.def_id()) | |||
| } | |||
|
|
|||
| crate fn is_local(&self, cache: &cache::Cache) -> bool { | |||
| self.inner_impl().for_.def_id(cache).map_or(true, |d| cache.paths.contains_key(&d)) | |||
There was a problem hiding this comment.
This function name is not accurate (see #86798). What do you intend for the behavior to be?
Sure, won't have time today but I'll review in the next two days. |
camelid
added
the
T-rustdoc
|
I kind of think we should just remove the implementors sidebar section; see #91118 (comment). |
Just for the record, I think the current one is fine. There is a discussion going on zulip about this PR here. Don't hesitate to come by to give your opinion as well @aDotInTheVoid. |
What do you mean by "the current one"? Do you mean having just the header and nothing below it? (As I said on Zulip, I'm fine with keeping the header, as long as it's clear that it's not supposed to have anything below it.) |
|
Yes, it's what I meant. It has a different style than the others sidebar headers but I guess it's not obvious, so if we keep it, we'll need to re-design it to fix it. |
|
☔ The latest upstream changes (presumably #91291) made this pull request unmergeable. Please resolve the merge conflicts. |
JohnCSimon
added
S-waiting-on-review
|
With the new sidebar design, this isnt under consideration |
Fixes #91118
@rustbot: modify labels: +A-rustdoc +A-rustdoc-ui +T-rustdoc
r? @jyn514