Skip to content
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

make Size and Align debug-printing a bit more compact #96581

Merged
merged 3 commits into from
May 7, 2022

Conversation

RalfJung
Copy link
Member

In particular in {:#?}-mode, these take up a lot of space, so I think this is the better alternative (even though it is a bit longer in {:?} mode, I think it is still more readable).

We could make it even smaller by deviating further from what the actual code looks like, e.g. via something like Size(4 bytes). Not sure what people would think about that?

Cc @oli-obk

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 30, 2022
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 30, 2022
// This is debug-printed a lot in larger structs, don't waste too much space there
impl fmt::Debug for Align {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "Align::from_bytes({})", self.bytes())
Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW, this is not actually runnable code -- the result of from_bytes needs to be unwraped.

@rust-log-analyzer

This comment has been minimized.

@@ -50,7 +50,7 @@ fn main() -> () {
_6 = const "D"; // scope 0 at $DIR/uninhabited_enum_branching.rs:27:21: 27:24
// mir::Constant
// + span: $DIR/uninhabited_enum_branching.rs:27:21: 27:24
// + literal: Const { ty: &str, val: Value(Slice { data: Allocation { bytes: [68], relocations: Relocations(SortedMap { data: [] }), init_mask: InitMask { blocks: [1], len: Size { raw: 1 } }, align: Align { pow2: 0 }, mutability: Not, extra: () }, start: 0, end: 1 }) }
// + literal: Const { ty: &str, val: Value(Slice { data: Allocation { bytes: [68], relocations: Relocations(SortedMap { data: [] }), init_mask: InitMask { blocks: [1], len: Size::from_bytes(1) }, align: Align::from_bytes(1), mutability: Not, extra: () }, start: 0, end: 1 }) }
Copy link
Member Author

Choose a reason for hiding this comment

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

Wow this looks terrible... I think we are better off just not printing all those details.^^

Copy link
Contributor

Choose a reason for hiding this comment

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

In mir dumps, definitely, I'd be wary about the regular debug impls tho, as we want all that info in case we print it in tracing statements I'd think

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's why in my last commit I changed MIR dumps, not the debug impls.

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented May 1, 2022

We could make it even smaller by deviating further from what the actual code looks like, e.g. via something like Size(4 bytes). Not sure what people would think about that?

All the usages I saw were redundantly listing the type name, when the field it was printed for was already obvious.

Maybe just print 4 bytes?

@petrochenkov
Copy link
Contributor

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned petrochenkov May 1, 2022
@RalfJung
Copy link
Member Author

RalfJung commented May 1, 2022

All the usages I saw were redundantly listing the type name, when the field it was printed for was already obvious.

Maybe just print 4 bytes?

Hm, I feel like debug output should be reasonably self-descriptive... I'll see how Align(N bytes) looks.

@oli-obk
Copy link
Contributor

oli-obk commented May 1, 2022

Hm, I feel like debug output should be reasonably self-descriptive

I find align: 8 bytes very self descriptive, but I guess just dumping a size/align directly would lack some info

@bors
Copy link
Contributor

bors commented May 4, 2022

☔ The latest upstream changes (presumably #94775) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung
Copy link
Member Author

RalfJung commented May 6, 2022

Yeah I think it should say the type.

I updated the PR accordingly.

@oli-obk
Copy link
Contributor

oli-obk commented May 6, 2022

@bors r+

@bors
Copy link
Contributor

bors commented May 6, 2022

📌 Commit d455752 has been approved by oli-obk

@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 6, 2022
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 6, 2022
make Size and Align debug-printing a bit more compact

In particular in `{:#?}`-mode, these take up a lot of space, so I think this is the better alternative (even though it is a bit longer in `{:?}` mode, I think it is still more readable).

We could make it even smaller by deviating further from what the actual code looks like, e.g. via something like `Size(4 bytes)`. Not sure what people would think about that?

Cc `@oli-obk`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 6, 2022
make Size and Align debug-printing a bit more compact

In particular in `{:#?}`-mode, these take up a lot of space, so I think this is the better alternative (even though it is a bit longer in `{:?}` mode, I think it is still more readable).

We could make it even smaller by deviating further from what the actual code looks like, e.g. via something like `Size(4 bytes)`. Not sure what people would think about that?

Cc ``@oli-obk``
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 6, 2022
make Size and Align debug-printing a bit more compact

In particular in `{:#?}`-mode, these take up a lot of space, so I think this is the better alternative (even though it is a bit longer in `{:?}` mode, I think it is still more readable).

We could make it even smaller by deviating further from what the actual code looks like, e.g. via something like `Size(4 bytes)`. Not sure what people would think about that?

Cc ```@oli-obk```
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 7, 2022
make Size and Align debug-printing a bit more compact

In particular in `{:#?}`-mode, these take up a lot of space, so I think this is the better alternative (even though it is a bit longer in `{:?}` mode, I think it is still more readable).

We could make it even smaller by deviating further from what the actual code looks like, e.g. via something like `Size(4 bytes)`. Not sure what people would think about that?

Cc ````@oli-obk````
bors added a commit to rust-lang-ci/rust that referenced this pull request May 7, 2022
…laumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#96581 (make Size and Align debug-printing a bit more compact)
 - rust-lang#96636 (Fix jump to def regression)
 - rust-lang#96760 (diagnostics: port more diagnostics to derive + add support for `Vec` fields)
 - rust-lang#96788 (Improve validator around field projections and checked bin ops)
 - rust-lang#96805 (Change eslint rules from configuration comments to configuration file)
 - rust-lang#96807 (update Miri)
 - rust-lang#96811 (Fix a minor typo in the description of Formatter)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c29f857 into rust-lang:master May 7, 2022
@rustbot rustbot added this to the 1.62.0 milestone May 7, 2022
@RalfJung RalfJung deleted the debug-size-align branch May 12, 2022 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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