Skip to content

Rustdoc: Report Layout of enum variants - #86263

Merged
bors merged 5 commits into
rust-lang:masterfrom
fee1-dead-contrib:rustdoc-layout-variants
Sep 8, 2021
Merged

Rustdoc: Report Layout of enum variants#86263
bors merged 5 commits into
rust-lang:masterfrom
fee1-dead-contrib:rustdoc-layout-variants

Conversation

@fee1-dead

@fee1-deadfee1-dead commented Jun 13, 2021

Copy link
Copy Markdown
Member

Followup of #83501, Fixes#86253.

cc @camelid

@rustbot label A-rustdoc

@rust-highfive

Copy link
Copy Markdown
Contributor

r? @ollie27

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2021
@rust-log-analyzer

This comment has been minimized.

@fee1-dead
fee1-deadforce-pushed the rustdoc-layout-variants branch from a6747c4 to 7c87a39CompareJune 13, 2021 10:17
Comment threadsrc/librustdoc/html/render/print_item.rs Outdated
@rust-log-analyzer

This comment has been minimized.

@fee1-dead
fee1-deadforce-pushed the rustdoc-layout-variants branch from 7c87a39 to 4a2e8adCompareJune 13, 2021 10:31
Comment threadsrc/librustdoc/html/render/print_item.rs Outdated
@fee1-dead
fee1-deadforce-pushed the rustdoc-layout-variants branch from 4a2e8ad to 1ab9596CompareJune 14, 2021 02:46
@rustbotrustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jun 21, 2021
@fee1-deadfee1-dead changed the title Report Layout of enum variantsRustdoc: Report Layout of enum variantsJun 23, 2021

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.

Can you test what happens for the empty case? Does it show 0 bytes or unsized? I think ideally it would show "uninhabited" or something.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It shows 0 bytes. I think it would be more suitable for a separate PR as a followup of the original PR.

Comment threadsrc/librustdoc/html/render/print_item.rs Outdated
Comment threadsrc/librustdoc/html/render/print_item.rs Outdated
@jyn514jyn514 added the requires-nightly This issue requires a nightly compiler in some way. When possible, use a F-* label instead. label Jul 8, 2021
@rust-log-analyzer

This comment has been minimized.

@jyn514jyn514 left a comment

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.

r=me with the test case updated

Comment threadsrc/test/rustdoc/type-layout.rs Outdated
@jyn514jyn514 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 Jul 9, 2021
@jyn514jyn514 assigned jyn514 and unassigned ollie27Jul 9, 2021
Comment threadsrc/librustdoc/html/render/print_item.rs Outdated

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.

What about Variants::Single?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Then it wouldn't matter because its size is already reported previously.

@pickfire

Copy link
Copy Markdown
Contributor

@fee1-dead Do you have a sample screenshot on how it looks like?

Comment threadsrc/librustdoc/html/render/print_item.rs Outdated
@fee1-dead

Copy link
Copy Markdown
MemberAuthor

@fee1-dead Do you have a sample screenshot on how it looks like?

image

@rustbot label -S-waiting-on-author S-waiting-on-review

@rustbotrustbot 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 Jul 10, 2021
@jyn514

Copy link
Copy Markdown
Member

@fee1-dead hmm, that doesn't seem super helpful - I assume those are all 0 because they don't have fields? Could we avoid showing the size in that case and only report it for fields?

Actually at that point I'm not really sure how much purpose this serves - why wouldn't the user just look at the size shown for the type in the field?

@jyn514jyn514 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 11, 2021
@jyn514

jyn514 commented Aug 30, 2021

Copy link
Copy Markdown
Member

@camelid do you have opinions on #86263 (comment) ?

@camelid

Copy link
Copy Markdown
Member

@camelid do you have opinions on #86263 (comment) ?

I find it annoying to have to click on each field's type to try to manually compute its size, especially if the type is something like Option<(DefId, Foo)>, where I have to manually add up its components, try to figure out what niches there are, and estimate alignment. And it gets especially bothersome for types with many variants like clean::Type.

I think it'd be useful to have a summary of the sizes of each variant.

Is that what you were asking about, or was it something else?

@jyn514

Copy link
Copy Markdown
Member

especially if the type is something like Option<(DefId, Foo)>

👍 makes sense, I hadn't thought about anonymous types. Ok, I think this makes sense then.

@fee1-dead
fee1-deadforce-pushed the rustdoc-layout-variants branch from ae9e562 to 5f1505eCompareAugust 31, 2021 05:59
@camelidcamelid 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 Aug 31, 2021
Comment threadsrc/librustdoc/html/render/print_item.rs
Comment threadsrc/test/rustdoc/type-layout.rs
// @has type_layout/enum.Variants.html 'Size: '
// @has - '2 bytes'
// @has - '<code>A</code>: 0 bytes'
// @has - '<code>B</code>: 1 byte'

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.

Just to confirm: this checks that this "text" is in the raw HTML directly, right?

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.

What's odd is I tried @has - '<strong>Size:</strong> 8 bytes on master on an existing test case, but it failed. I'm not sure why these tests are passing.

@camelidcamelid 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 Sep 2, 2021
@rustbotrustbot 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 Sep 6, 2021
@fee1-dead
fee1-deadforce-pushed the rustdoc-layout-variants branch from 5065be6 to c0451f7CompareSeptember 6, 2021 07:11
@camelid

Copy link
Copy Markdown
Member

Thanks!

@bors r+ rollup

@bors

bors commented Sep 6, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit c0451f7 has been approved by camelid

@borsbors 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 Sep 6, 2021
@jackh726jackh726 mentioned this pull request Sep 8, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 8, 2021
Rollup of 9 pull requests
Successful merges:
- rust-lang#86263 (Rustdoc: Report Layout of enum variants)
- rust-lang#88541 (Add regression test for rust-lang#74400)
- rust-lang#88553 (Improve diagnostics for unary plus operators (rust-lang#88276))
- rust-lang#88594 (More symbolic doc aliases)
- rust-lang#88648 (Correct “copies” to “moves” in `<Option<T> as From<T>>::from` doc, and other copyediting)
- rust-lang#88691 (Add a regression test for rust-lang#88649)
- rust-lang#88694 (Drop 1.56 stabilizations from 1.55 release notes)
- rust-lang#88712 (Fix docs for `uX::checked_next_multiple_of`)
- rust-lang#88726 (Fix typo in `const_generics` replaced with `adt_const_params` note)
Failed merges:
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit 2f2aed1 into rust-lang:masterSep 8, 2021
@rustbotrustbot added this to the 1.57.0 milestone Sep 8, 2021
bors added a commit to rust-lang/rust-analyzer that referenced this pull request May 18, 2023
Add layout info for enum variant and locals
The size of enum variant is what rustdoc shows (rust-lang/rust#86263). I also added layout info for locals since it helps finding size of unnameable types like closures inside other structs or impl traits.
@fee1-dead
fee1-dead deleted the rustdoc-layout-variants branch July 19, 2024 09:29
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-layoutArea: Memory layout of typesrequires-nightlyThis issue requires a nightly compiler in some way. When possible, use a F-* label instead.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-rustdocRelevant to the rustdoc team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rustdoc: report layout of enum variants

13 participants

@fee1-dead@rust-highfive@rust-log-analyzer@pickfire@jyn514@camelid@bors@GuillaumeGomez@Urgau@LeSeulArtichaut@JohnCSimon@ollie27@rustbot