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

Implement Debug for std::path::{Components,Iter}. #36101

Merged
merged 2 commits into from
Aug 31, 2016
Merged

Implement Debug for std::path::{Components,Iter}. #36101

merged 2 commits into from
Aug 31, 2016

Conversation

frewsxcv
Copy link
Member

No description provided.

Stebalien
Stebalien reviewed Aug 29, 2016
View reviewed changes
src/libstd/path.rs
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
let components = self.as_path()
.components()
.collect::<Vec<_>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a shame to have to allocate on debug. You should be able to do this using debug_list and a helper struct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I can switch this to debug_list to avoid the allocation, though we lose the "Components" title. I wonder if there is value in adding a debug_iter that offers name and iterator params.

Copy link
Contributor

@Stebalien Stebalien Aug 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Make a helper struct that implements debug and outputs [...]. That is,

f.debug_tuple("Components").field(&DebugHelper(self)).finish();

where DebugHelper implements Debug.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 29, 2016
@alexcrichton alexcrichton self-assigned this Aug 29, 2016
@alexcrichton
Copy link
Member

Thanks @frewsxcv! I wonder if perhaps the same implementation should be used for the other iterators in this module?

@frewsxcv frewsxcv force-pushed the debug-path-components branch from 8ce02e1 to 8cc3070 Compare August 30, 2016 13:29
@frewsxcv frewsxcv changed the title Implement Debug for std::path::Components. Implement Debug for std::path::{Components,Iter}. Aug 30, 2016
@frewsxcv
Copy link
Member Author

Latest commits no longer allocate Vecs. I also implemented Debug for std::path::Iter.

@frewsxcv frewsxcv force-pushed the debug-path-components branch from 8cc3070 to b8ebf77 Compare August 30, 2016 13:36
alexcrichton
alexcrichton reviewed Aug 30, 2016
View reviewed changes
src/libstd/path.rs
@@ -639,6 +639,25 @@ pub struct Iter<'a> {
inner: Components<'a>,
}

#[stable(feature = "path_components_debug", since = "")]
Copy link
Member

Choose a reason for hiding this comment

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

This since should be 1.13.0 I believe

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in the latest force push.

@frewsxcv frewsxcv force-pushed the debug-path-components branch from b8ebf77 to e118d4c Compare August 30, 2016 16:40
@alexcrichton
Copy link
Member

@bors: r+ e118d4c

@alexcrichton alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 30, 2016
@sophiajt
Copy link
Contributor

@bors rollup

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Aug 30, 2016
Rollup merge of rust-lang#36101 - frewsxcv:debug-path-components, r=a…
1404423 
…lexcrichton

Implement `Debug` for `std::path::{Components,Iter}`.

None
@sophiajt sophiajt mentioned this pull request Aug 30, 2016
Closed
@sophiajt
Copy link
Contributor

sophiajt commented Aug 30, 2016 •
edited
Loading

@bors r-

I'm seeing this failure on build_bot (auto-win-msvc-64-opt-rustbuild), and it seems related:

failures:
    path::tests::test_iter_debug

test result: FAILED. 736 passed; 1 failed; 4 ignored; 0 measured

If that's not related I can re-approve.

@frewsxcv frewsxcv force-pushed the debug-path-components branch from e118d4c to 268b3f5 Compare August 30, 2016 23:57
@frewsxcv
Copy link
Member Author

More info:

thread '<unnamed>' panicked at 'assertion failed: `(left == right)` (left: `"Iter([\"/\", \"tmp\"])"`, right: `"Iter([\"\\\\\", \"tmp\"])"`)', C:\bot\slave\auto-win-msvc-64-opt-rustbuild\build\src\libstd\path.rs:3554

I changed that test to only run on unix now.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Aug 31, 2016

📌 Commit 268b3f5 has been approved by alexcrichton

@sophiajt sophiajt mentioned this pull request Aug 31, 2016
Closed
sophiajt pushed a commit to sophiajt/rust that referenced this pull request Aug 31, 2016
Rollup merge of rust-lang#36101 - frewsxcv:debug-path-components, r=a…
fc80e7d 
…lexcrichton

Implement `Debug` for `std::path::{Components,Iter}`.

None
sophiajt pushed a commit to sophiajt/rust that referenced this pull request Aug 31, 2016
Rollup merge of rust-lang#36101 - frewsxcv:debug-path-components, r=a…
1048ea2 
…lexcrichton

Implement `Debug` for `std::path::{Components,Iter}`.

None
@sophiajt sophiajt mentioned this pull request Aug 31, 2016
Merged
bors added a commit that referenced this pull request Aug 31, 2016
@bors
Auto merge of #36166 - jonathandturner:rollup, r=jonathandturner
7a187c3 
Rollup of 16 pull requests

- Successful merges: #35758, #35926, #36050, #36079, #36085, #36089, #36101, #36130, #36134, #36135, #36136, #36140, #36141, #36147, #36148, #36165
- Failed merges:
@bors bors merged commit 268b3f5 into rust-lang:master Aug 31, 2016
@frewsxcv frewsxcv deleted the debug-path-components branch August 31, 2016 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@frewsxcv @alexcrichton @sophiajt @bors @Stebalien