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

DSTify Show #18431

Merged
merged 1 commit into from
Oct 31, 2014
Merged

DSTify Show #18431

merged 1 commit into from
Oct 31, 2014

Conversation

japaric
Copy link
Member

@japaric japaric commented Oct 29, 2014

r? @aturon
cc #16918

@@ -168,7 +168,7 @@ impl<'a> Show for Arguments<'a> {
/// When a format is not otherwise specified, types are formatted by ascribing
/// to this trait. There is not an explicit way of selecting this trait to be
/// used for formatting, it is only if no other format is specified.
pub trait Show {
pub trait Show for Sized? {
Copy link
Member

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
Member Author

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
Member

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

Thanks @japaric!

@huonw
Copy link
Member

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

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
Member

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
Member Author

japaric commented Oct 31, 2014

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

@aturon re-r?

@aturon
Copy link
Member

aturon commented Oct 31, 2014

Looks good modulo the nits that @alexcrichton pointed out.

@japaric
Copy link
Member 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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants