Skip to content

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

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

Merged
merged 1 commit into from
Jun 2, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 30 additions & 24 deletions library/core/src/fmt/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2233,35 +2233,41 @@ impl Display for char {
#[stable(feature = "rust1", since = "1.0.0")]
impl<T: ?Sized> Pointer for *const T {
fn fmt(&self, f: &mut Formatter<'_>) -> Result {
/// Since the formatting will be identical for all pointer types, use a non-monomorphized
/// implementation for the actual formatting to reduce the amount of codegen work needed
fn inner(ptr: *const (), f: &mut Formatter<'_>) -> Result {
let old_width = f.width;
let old_flags = f.flags;

// The alternate flag is already treated by LowerHex as being special-
// it denotes whether to prefix with 0x. We use it to work out whether
// or not to zero extend, and then unconditionally set it to get the
// prefix.
if f.alternate() {
f.flags |= 1 << (FlagV1::SignAwareZeroPad as u32);

if f.width.is_none() {
f.width = Some((usize::BITS / 4) as usize + 2);
}
}
f.flags |= 1 << (FlagV1::Alternate as u32);
// Cast is needed here because `.addr()` requires `T: Sized`.
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.

}
}

let ret = LowerHex::fmt(&(ptr.addr()), f);
/// Since the formatting will be identical for all pointer types, use a non-monomorphized
/// implementation for the actual formatting to reduce the amount of codegen work needed.
///
/// This uses `ptr_addr: usize` and not `ptr: *const ()` to be able to use this for
/// `fn(...) -> ...` without using [problematic] "Oxford Casts".
///
/// [problematic]: https://github.com/rust-lang/rust/issues/95489
pub(crate) fn pointer_fmt_inner(ptr_addr: usize, f: &mut Formatter<'_>) -> Result {
let old_width = f.width;
let old_flags = f.flags;

f.width = old_width;
f.flags = old_flags;
// The alternate flag is already treated by LowerHex as being special-
// it denotes whether to prefix with 0x. We use it to work out whether
// or not to zero extend, and then unconditionally set it to get the
// prefix.
if f.alternate() {
f.flags |= 1 << (FlagV1::SignAwareZeroPad as u32);

ret
if f.width.is_none() {
f.width = Some((usize::BITS / 4) as usize + 2);
}

inner(*self as *const (), f)
}
f.flags |= 1 << (FlagV1::Alternate as u32);

let ret = LowerHex::fmt(&ptr_addr, f);

f.width = old_width;
f.flags = old_flags;

ret
}

#[stable(feature = "rust1", since = "1.0.0")]
Expand Down
14 changes: 2 additions & 12 deletions library/core/src/ptr/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1831,24 +1831,14 @@ macro_rules! fnptr_impls_safety_abi {
#[stable(feature = "fnptr_impls", since = "1.4.0")]
impl<Ret, $($Arg),*> fmt::Pointer for $FnTy {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
// HACK: The intermediate cast as usize is required for AVR
// so that the address space of the source function pointer
// is preserved in the final function pointer.
//
// https://github.com/avr-rust/rust/issues/143
fmt::Pointer::fmt(&(*self as usize as *const ()), f)
fmt::pointer_fmt_inner(*self as usize, f)
}
}

#[stable(feature = "fnptr_impls", since = "1.4.0")]
impl<Ret, $($Arg),*> fmt::Debug for $FnTy {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
// HACK: The intermediate cast as usize is required for AVR
// so that the address space of the source function pointer
// is preserved in the final function pointer.
//
// https://github.com/avr-rust/rust/issues/143
fmt::Pointer::fmt(&(*self as usize as *const ()), f)
fmt::pointer_fmt_inner(*self as usize, f)
}
}
}
Expand Down