-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Stabilize debug_closure_helpers
#146099
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
base: master
Are you sure you want to change the base?
Stabilize debug_closure_helpers
#146099
Conversation
debug_closure_helpers
☔ The latest upstream changes (presumably #146636) made this pull request unmergeable. Please resolve the merge conflicts. |
Once the FCP is complete, I think it might make sense to merge #145915 first and then merge this on top of that. |
pub fn field_with<F>(&mut self, name: &str, value_fmt: F) -> &mut Self | ||
where | ||
F: FnOnce(&mut fmt::Formatter<'_>) -> fmt::Result, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the closure as a generic parameter (and not type-erasing it internally) leads to a lot of code bloat compared to the corresponding stable APIs that take &dyn Debug
as value to format. According to cargo +nightly llvm-lines
a simple program formatting a struct with two fields generates 694 lines of LLVM IR while the same program using field()
generates 42 lines. So at least with the current implementation, these functions have an annoying downside compared to e.g. .field("foo", &fmt::from_fn(|f| ...)
I think this can be addressed after stabilization, without changing the signatures. But I wanted to flag it so the reviewer can think about it as well. It's not entirely obvious to me for the helpers that deal in FnOnce
. I'm aware of one workaround (put the closure into an Option
, then pass a &dyn FnMut
that unwraps this option to call the original impl FnOnce
) but it's not quite zero cost in several dimensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be solved by putting the main logic back into field()
, and having field_with
be an #[inline]
wrapper around it?
I don't want to send in any PRs that might cause Git conflicts with the stabilization, but if the public API is good then I'd be happy to experiment with internal reorganization / optimization after both have landed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
field
takes a &dyn Debug
. How do you invoke a FnOnce
or FnMut
from the &self
argument of Debug::fmt
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cell<Option<F>>
, then .replace(None).unwrap()
? It doesn't need to survive more than one call.
Resolves #117729. The tracking issue still needs an FCP, but I'm hoping that creating a stabilization PR will prompt one.
r? libs-api