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

Add key and value methods to DebugMap #2696

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@KodrAus
Copy link
Contributor

commented May 1, 2019

Rendered

Add two new methods to std::fmt::DebugMap for writing the key and value part of a map entry separately:

impl<'a, 'b: 'a> DebugMap<'a, 'b> {
    pub fn key(&mut self, key: &dyn Debug) -> &mut Self;
    pub fn value(&mut self, value: &dyn Debug) -> &mut Self;
}
# Drawbacks
[drawbacks]: #drawbacks

The proposed `key` and `value` methods are't immediately useful for `Debug` implementors that are able to call `entry` instead. This creates a decision point where there wasn't one before. The proposed implementation is also going to be less efficient than the one that exists now because it introduces a few conditionals.

This comment has been minimized.

Copy link
@Centril

Centril May 1, 2019

Contributor

Regarding efficiency, I would suggest testing this out with a PR and then doing a timer build to see how much perf regresses (that just affects compile times but it's better than nothing). With that data we can evaluate the drawback better.

This comment has been minimized.

Copy link
@KodrAus

KodrAus May 1, 2019

Author Contributor

I opened up a PR (rust-lang/rust#60458) that we can use to check correctness and get some timings 👍

I don't expect the difference to be significant, since it amounts to a few conditionals but it'll be good to verify!

This comment has been minimized.

Copy link
@KodrAus

KodrAus May 13, 2019

Author Contributor

Alrighty, I posted a quick benchmark in the PR, but also inlining the results here:

Before (current .entry()) After (.key().value())
36 ns/iter (+/- 7) 44 ns/iter (+/- 2)

- Write an alternative implementation of the format builders. The output from this alternative implementation would need to be kept reasonably in-sync with the one in the standard library. It doesn't change very frequently, but does from time to time. It would also have to take the same care as the standard library implementation to retain formatting flags when working with entries.
- Buffer keys and format them together with values when the whole entry is available. Unless the key is guaranteed to live until the value is supplied (meaning it probably needs to be `'static`) then the key will need to be formatted into a string first. This means allocating (though the cost could be amortized over the whole map) and potentially losing formatting flags when buffering.

This comment has been minimized.

Copy link
@Centril

Centril May 1, 2019

Contributor

A third option here is to simply not error when keys and values come alone. That might actually be useful in some weird semi-map semi-array cases. That would also be more efficient?

This comment has been minimized.

Copy link
@KodrAus

KodrAus May 1, 2019

Author Contributor

Hmm, that's an interesting thought 🤔 I think that might just mask incorrect implementations though. I imagine we'd be tracking the same additional state too so that we knew when a key or value called 'out of order' should be formatted in a set-like way rather than a map-like way.

This comment has been minimized.

Copy link
@Centril

Centril May 1, 2019

Contributor

Yeah quite possibly.
Would be good to record this in the text of the RFC in any case. :)

pub fn finish(&mut self) -> fmt::Result {
// Make sure there isn't a partial entry
if self.has_key {
return Err(fmt::Error);

This comment has been minimized.

Copy link
@ollie27

ollie27 May 19, 2019

Creating a fmt::Error like this isn't allowed. From https://doc.rust-lang.org/nightly/std/fmt/index.html#formatting-traits:

a formatting implementation must and may only return an error if the passed-in Formatter returns an error.

As this indicates a programming error I think an explicit panic here would be correct.

This comment has been minimized.

Copy link
@KodrAus

KodrAus May 19, 2019

Author Contributor

Sure! A panic seems reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.