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 #60458

Merged
merged 2 commits into from
Jul 9, 2019
Merged

Add key and value methods to DebugMap #60458

merged 2 commits into from
Jul 9, 2019

Conversation

KodrAus
Copy link
Contributor

@KodrAus KodrAus commented May 1, 2019

Implementation PR for an active (not approved) RFC: rust-lang/rfcs#2696.

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;
}

I want to do this so that I can write a serde::Serializer that forwards to our format builders, so that any T: Serialize can also be treated like a T: Debug.

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @rkruppe

(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 1, 2019
@KodrAus KodrAus added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 1, 2019
src/libcore/fmt/builders.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

buf: &'a mut (dyn fmt::Write + 'a),
struct PadAdapter<'buf, 'state> {
buf: &'buf mut (dyn fmt::Write + 'buf),
state: &'state mut PadAdapterState,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shifting the state of the PadAdapter into a dedicated struct here lets value pick up where key left off without having to guess what the state was before.

@GuillaumeGomez
Copy link
Member

@rust-lang/infra: seems like a condition is broken:

Some changes occurred in HTML/CSS.

cc @GuillaumeGomez

@pietroalbini
Copy link
Member

@GuillaumeGomez
Copy link
Member

Ah indeed! My bad.

@KodrAus
Copy link
Contributor Author

KodrAus commented May 13, 2019

Very quick, very dirty benchmark:

#![feature(test)]

extern crate test;

use std::{
    cell::Cell,
    fmt::{self, Write},
};

struct NoOpWrite;

impl Write for NoOpWrite {
    fn write_str(&mut self, _: &str) -> fmt::Result {
        Ok(())
    }
}

#[bench]
fn debugmap_entry(b: &mut test::Bencher) {
    struct Map<'a>(Cell<Option<&'a mut test::Bencher>>);

    impl<'a> fmt::Debug for Map<'a> {
        fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
            let mut map = f.debug_map();

            let b = self.0.take().unwrap();
            b.iter(|| {
                map.entry(&"a", &42);
            });

            map.finish()
        }
    }

    let _ = write!(NoOpWrite, "{:?}", Map(Cell::new(Some(b))));
}

On my machine, running this on the current nightly and this PR yields:

Before After
36 ns/iter (+/- 7) 44 ns/iter (+/- 2)

So there's a bit of a performance impact like you'd expect, but I don't think it's too significant, and if it is we can inline the implementation in entry.

@KodrAus
Copy link
Contributor Author

KodrAus commented May 17, 2019

r? @alexcrichton

@rust-highfive

This comment has been minimized.

@alexcrichton
Copy link
Member

This all looks great to me! I think this is still waiting on the RFC, though, right?

(FWIW I wouldn't personally consider this as requiring an RFC, but I don't mind waiting if one's open!)

@KodrAus
Copy link
Contributor Author

KodrAus commented May 22, 2019

Thanks! Yeh, there's an RFC for this at the moment, so I'm also happy to let this sit and wait on that.

@Centril Centril added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 10, 2019
@KodrAus KodrAus added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. B-unstable Blocker: Implemented in the nightly compiler and unstable. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jul 7, 2019
@KodrAus
Copy link
Contributor Author

KodrAus commented Jul 7, 2019

Alrighty, the RFC has been merged and I've updated the stability attributes to point to the tracking issue.

I think this should be ready to go now!

@alexcrichton
Copy link
Member

r=me, thanks!

Looks like CI may be failing though?

@KodrAus
Copy link
Contributor Author

KodrAus commented Jul 9, 2019

Looks like we're all good!

@bors r=alexcrichton

@Centril Centril closed this Jul 9, 2019
@Centril Centril reopened this Jul 9, 2019
@Centril
Copy link
Contributor

Centril commented Jul 9, 2019

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Jul 9, 2019

📌 Commit 70d630f has been approved by alexcrichton

@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 Jul 9, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 9, 2019
…hton

Add key and value methods to DebugMap

Implementation PR for an active (not approved) RFC: rust-lang/rfcs#2696.

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

```rust
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;
}
```

I want to do this so that I can write a `serde::Serializer` that forwards to our format builders, so that any `T: Serialize` can also be treated like a `T: Debug`.
bors added a commit that referenced this pull request Jul 9, 2019
Rollup of 4 pull requests

Successful merges:

 - #60458 (Add key and value methods to DebugMap)
 - #62090 (typeck: merge opaque type inference logic)
 - #62403 (Replace SliceConcatExt trait with inherent methods and SliceConcat helper trait)
 - #62494 (Remove unused dependencies)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Jul 9, 2019

⌛ Testing commit 70d630f with merge 909f5a0...

@bors bors merged commit 70d630f into rust-lang:master Jul 9, 2019
@KodrAus KodrAus deleted the debug_map_entry branch July 15, 2019 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants