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.

Sign up for GitHub

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

Jump to bottom

Be a little nicer with casts when formatting fn pointers #97420

Merged
merged 1 commit into from
Jun 2, 2022
Merged

Be a little nicer with casts when formatting fn pointers #97420

merged 1 commit into from
Jun 2, 2022

Conversation

WaffleLapkin
Copy link
Member

This removes a fn(...) -> ... -> usize -> *const () -> usize cast. cc #95489.

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 26, 2022
@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 May 26, 2022
WaffleLapkin
WaffleLapkin commented May 26, 2022
View reviewed changes
library/core/src/fmt/mod.rs
}
}
f.flags |= 1 << (FlagV1::Alternate as u32);
pointer_fmt_inner((*self as *const ()).addr(), f)
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if this as *const () does anything.

@jam1garner do you have an idea how this might affect anything/how to test this?

Copy link
Member

Choose a reason for hiding this comment

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

Can you say more about why it's here? I would expect this code to be self.addr(), I 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.

I've left it here from the previous version in which it was used to erase the type. But since this gets the addr, I guess it's useless now.

Copy link
Contributor

@jam1garner jam1garner May 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The reason it is there is in order to erase the type to avoid monomorphizing the impl, since the debug/pointer formatting doesn't care about the type, just the address.

The reason this matters is that if you derive(Debug) for bindgen'd bindings involving lots of pointer fields it generates an obscene amount of code, and severely hurts compile times.

I agree that it should be entirely useless now in the presence of .addr()! (I also do not know why T: Sized is needed)

Copy link
Member

Choose a reason for hiding this comment

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

I think the T: Sized bound on addr exists to make discarding the metadata explicit.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wander is it makes sense then to have a .discard_meta(): *const ()? it would play nicely with other methods like said addr. Though I don't really see a value in making discarding metadata explicit (at least for addr).

Copy link
Member

Choose a reason for hiding this comment

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

addr() already discards other information so I don't disagree -- however, map_addr should then probably also be made to support ?Sized.

@Gankra designed those APIs so she might better be able to explain why addr requires Sized.

@Mark-Simulacrum
Copy link
Member

r=me with the seemingly useless cast to a unit raw ptr removed.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@WaffleLapkin
Copy link
Member Author

What, why does addr require T: Sized 🤔

@Mark-Simulacrum
Copy link
Member

Ok, let's leave the cast in for now and leave a comment on why.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@WaffleLapkin
Copy link
Member Author

Done 😅

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 1, 2022

📌 Commit ac5c15d has been approved by Mark-Simulacrum

@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 Jun 1, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 1, 2022
@Dylan-DPC
Rollup merge of rust-lang#97420 - WaffleLapkin:no_oxford_casts_qqq, r…
e7288d2 
…=Mark-Simulacrum

Be a little nicer with casts when formatting `fn` pointers

This removes a `fn(...) -> ...` -> `usize` -> `*const ()` -> `usize` cast. cc rust-lang#95489.
@Dylan-DPC Dylan-DPC mentioned this pull request Jun 1, 2022
Closed
@RalfJung RalfJung mentioned this pull request Jun 1, 2022
Closed
@Dylan-DPC Dylan-DPC mentioned this pull request Jun 2, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 2, 2022
@bors
Auto merge of rust-lang#97654 - Dylan-DPC:rollup-w6zrzxf, r=Dylan-DPC
44e9516 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#97420 (Be a little nicer with casts when formatting `fn` pointers)
 - rust-lang#97450 ([RFC 2011] Basic compiler infrastructure)
 - rust-lang#97599 (Fix JSON reexport ICE)
 - rust-lang#97617 (Rustdoc anonymous reexports)
 - rust-lang#97636 (Revert rust-lang#96682.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0b2d48e into rust-lang:master Jun 2, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 2, 2022
@WaffleLapkin WaffleLapkin deleted the no_oxford_casts_qqq branch August 4, 2022 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

@jam1garner jam1garner jam1garner left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@WaffleLapkin @rust-highfive @Mark-Simulacrum @rust-log-analyzer @bors @RalfJung @jam1garner @rustbot