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

Modify collection's Debug output to resemble in their content only #22157

Merged
merged 2 commits into from
Feb 25, 2015
Merged

Modify collection's Debug output to resemble in their content only #22157

merged 2 commits into from
Feb 25, 2015

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Feb 10, 2015

r? @gankro

@tbu-
Copy link
Contributor Author

tbu- commented Feb 10, 2015

reem voiced some concerns on IRC (2015-02-10 22:00:00 +0100):

[22:16:45] <reem> eibwen: I thought the decision was to keep types in Debug output
[22:17:19] <eibwen> oh, was it?
[22:17:24] <eibwen> I'll dig up the RFC
[22:17:32] <eibwen> I thought it was the other way around
[22:18:15] <eibwen> reem: from the RFC: Focus on the *runtime* aspects of a type; repeating information such as suffixes for integer literals is not generally useful since that data is readily available from the type definition.
[22:19:03] <eibwen> reem: further down: Containers print using *some* notation that makes their type and contents clear. (Since we lack literals for all container types, this will be ad hoc).
[22:19:17] <eibwen> this is indicated by using [] for sequences and {} for sets/maps
[22:19:30] <reem> eibwen: [] or {} is not enough to tell me the tyep
[22:19:37] <reem> if I want to know the type I need a name too
[22:20:15] <eibwen> I might well have misunderstood the RFC, but I interpret it as "focus on the semantics of the type, not its implementation"
[22:20:49] <eibwen> which is: "sequence" for Vec, DList, etc. (the actual type can be seen in source code, the semantic type is there)
[22:21:00] <eibwen> or "map" for HashMap, TreeMap, etc.
[22:21:55] <bluss_> I like it. I think it composes better with derived Debug impls of custom types? (But those still use struct names(?))
[22:22:25] <reem> I still think the types should stay in the output
[22:22:47] <eibwen> I think the terser the output, the better
[22:22:52] <eibwen> as long as it's readable
[22:23:02] <eibwen> because more things fit on the screen
[22:23:25] <eibwen> (this has been an actual nuisance for me, if you nest a few structs, the debug output explodes)

@Gankra
Copy link
Contributor

Gankra commented Feb 10, 2015

Conventions issue, punting to r? @aturon

@rust-highfive rust-highfive assigned aturon and unassigned Gankra Feb 10, 2015
Gankra
Gankra reviewed Feb 10, 2015
View reviewed changes
src/libcollections/bit.rs
@@ -977,7 +977,7 @@ impl Ord for Bitv {
impl fmt::Debug for Bitv {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
for bit in self {
try!(write!(fmt, "{}", if bit { 1u32 } else { 0u32 }));
try!(write!(fmt, "{}", if bit { 1 } else { 0 }));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting case. A big binary string seems reasonable, I suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't change anything here though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah totally. Poking around it just got me thinking.

@Gankra
Copy link
Contributor

Gankra commented Feb 10, 2015

Seems to be missing fixes for BTree* if you meant to be exhaustive.

Also apparently BinaryHeap has no impl??

@tbu-
Copy link
Contributor Author

tbu- commented Feb 10, 2015

@gankro Implemented for BTree* as well.

@alexcrichton
Copy link
Member

Citing RFC 565

Containers print using some notation that makes their type and contents clear. (Since we lack literals for all container types, this will be ad hoc).

(just pointing out that this is currently intended)

@tbu-
Copy link
Contributor Author

tbu- commented Feb 11, 2015

@alexcrichton I understand that as: Containers print something that makes their semantic type clear ([] for sequence, e.g. Vec, DList, etc., {} for maps, sets, like HashSet, HashMap, ...).

@alexcrichton
Copy link
Member

I would personally be ok with that interpretation, and I'm in favor of this change.

@aturon
Copy link
Member

aturon commented Feb 11, 2015

@tbu- Thanks for this PR!

I'm also in favor of these changes and interpretation of the RFC, particularly since none of the current outputs can be used to reconstruct the collection.

(Aside: if we had macros for constructing each collection, you could imagine printing vec![1, 2] and hash_map["foo": 1, "bar": 2] etc.)

cc @seanmonstar @wycats

@tbu-
Copy link
Contributor Author

tbu- commented Feb 24, 2015

@aturon Any update? Should I rebase this?

@aturon
Copy link
Member

aturon commented Feb 24, 2015

@tbu- As I said in an earlier comment, I'm happy with these changes -- sorry nothing has happened since then!

If you rebase, please ping @alexcrichton since I am away on vacation this week.

@alexcrichton
Copy link
Member

Yes I'd be fine merging with a rebase

@tbu- tbu- force-pushed the pr_debug_collections branch from 3064e67 to 870ad3b Compare February 24, 2015 22:47
@tbu-
Copy link
Contributor Author

tbu- commented Feb 24, 2015

Rebased and make checked.

@alexcrichton
Copy link
Member

@bors: r+ 870ad3b

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 25, 2015
@Manishearth
Rollup merge of rust-lang#22157 - tbu-:pr_debug_collections, r=alexcr…
c950ee9 
…ichton

 r? @gankro
@bors
Copy link
Contributor

bors commented Feb 25, 2015

⌛ Testing commit 870ad3b with merge 5e5c9f6...

@bors
Copy link
Contributor

bors commented Feb 25, 2015

💔 Test failed - auto-linux-64-x-android-t

@Manishearth
Copy link
Member

(Not a real failure, happened because I forced the rollup through)

@Manishearth
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Feb 25, 2015

⚡ Previous build results are reusable. Rebuilding only auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt...

@bors
Copy link
Contributor

bors commented Feb 25, 2015

💔 Test failed - auto-win-32-nopt-t

@Manishearth
Copy link
Member

@bors: retry

(not a real failure)

@bors
Copy link
Contributor

bors commented Feb 25, 2015

⚡ Previous build results are reusable. Rebuilding only auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt...

@bors
Copy link
Contributor

bors commented Feb 25, 2015

💔 Test failed - auto-win-32-opt

@Manishearth
Copy link
Member

@bors: retry

(Please ignore the failures for now)

@bors
Copy link
Contributor

bors commented Feb 25, 2015

⚡ Previous build results are reusable. Rebuilding only auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt...

@bors
Copy link
Contributor

bors commented Feb 25, 2015

💔 Test failed - auto-win-64-nopt-t

@bors bors merged commit 870ad3b into rust-lang:master Feb 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@aturon aturon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tbu- @Gankra @alexcrichton @aturon @bors @Manishearth