Skip to content

DSTify Show#18431

Merged
bors merged 1 commit intorust-lang:masterfrom
japaric:show
Oct 31, 2014
Merged

DSTify Show#18431
bors merged 1 commit intorust-lang:masterfrom
japaric:show

Conversation

@japaric
Copy link
Contributor

@japaric japaric commented Oct 29, 2014

r? @aturon
cc #16918

alexcrichton
alexcrichton reviewed Oct 29, 2014
View reviewed changes
src/libcore/fmt/mod.rs
Copy link
Member

@alexcrichton alexcrichton Oct 29, 2014

Choose a reason for hiding this comment

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

This actually applies pretty generally to all the formatting traits, should they all get for Sized? as well?

Copy link
Contributor Author

@japaric japaric Oct 29, 2014

Choose a reason for hiding this comment

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

We could add Sized? to the other formatting traits in case someone wants to do impl Binary for [T] or something like that. I didn't see any use like that in the repo so I didn't add Sized? to the other formatting traits, but I guess it wouldn't hurt to do that.

@aturon thoughts?

Copy link
Contributor

@aturon aturon Oct 31, 2014

Choose a reason for hiding this comment

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

I'd vote to go ahead and add Sized? to those traits as well -- seems like there's little harm. Otherwise, this PR looks good to me!

@alexcrichton
Copy link
Member

alexcrichton commented Oct 29, 2014

Thanks @japaric!

@huonw
Copy link
Contributor

huonw commented Oct 29, 2014

Hm, does an unsized type implementing Show work correctly with the unsafe code inside format_args?

@alexcrichton
Copy link
Member

alexcrichton commented Oct 29, 2014

It should be ok with format_args starts out by taking a reference to all arguments in a match and then passing around &T while placing bounds on T. Did you have a specific worry in mind @huonw?

@huonw
Copy link
Contributor

huonw commented Oct 29, 2014

Nothing specific, just wanted to be sure our unsafe is correct and it sounds like it is fine, so that's good.

@japaric
Copy link
Contributor Author

japaric commented Oct 31, 2014

Added Sized? to all the other formatting traits. Rebased and squashed.

@aturon re-r?

@aturon
Copy link
Contributor

aturon commented Oct 31, 2014

Looks good modulo the nits that @alexcrichton pointed out.

@japaric
Copy link
Contributor Author

japaric commented Oct 31, 2014

Addressed @alexcrichton comments. I'm leaving the UFCS conversion for another PR since it requires the use of cfg(stage0) (format_args! seems to be hard wired to the secret_* functions on stage0)

re-r? @aturon

bors added a commit that referenced this pull request Oct 31, 2014
@bors
auto merge of #18431 : japaric/rust/show, r=alexcrichton
065caf3 
r? @aturon 
cc #16918
@bors bors closed this Oct 31, 2014
@bors bors merged commit eef7e97 into rust-lang:master Oct 31, 2014
@japaric japaric deleted the show branch November 1, 2014 05:28
lnicola added a commit to lnicola/rust that referenced this pull request Oct 29, 2024
@lnicola
Merge pull request rust-lang#18431 from lnicola/sync-from-rust
eae9d7a 
minor: Sync from downstream
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.

5 participants

@japaric @alexcrichton @huonw @aturon @bors