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 some missing Show implementations in libstd #12218

Closed
wants to merge 1 commit into from

Conversation

brendanzab
Copy link
Member

No description provided.

}

#[cfg(not(stage0))]
impl<T: fmt::Show> fmt::Show for ~[T] {
Copy link
Member

Choose a reason for hiding this comment

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

Could this delegate to the above one?

@alexcrichton
Copy link
Member

Why are all of these marked #[inline]? They're all called through virtual dispatch so I don't think that it'd be buying us much.

impl<'a> fmt::Show for Ascii {
#[inline]
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.buf.write([self.chr])
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps (self.chr as char).fmt(f) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

self.chr is a u8

Copy link
Member

Choose a reason for hiding this comment

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

With small primitives like this, though, it's nice to take the formatting flags into account (wherever possible). This is losing the formatting flags by writing directly, but if you call fmt(f) it'll take all the formatting flags into account properly (delegating elsewhere)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ohhh! Woops, for some reason I forgot that u8 as char was allowed.

@brendanzab
Copy link
Member Author

Updatated!

let table_str = format!("{}", table);

assert!(table_str == ~"{1: s2, 3: s4}" || table_str == ~"{3: s4, 1: s2}");
assert_eq!(format!("{}", empty), ~"{}");
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you actually run this test? I don't see how it can possibly pass. You're asserting that your ShowableStructs will print their values as s2, s4, but AFAIK they'll print as ShowableStruct { value: 2 }, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah woops. Fixing.

Copy link
Contributor

Choose a reason for hiding this comment

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

My recommendation is to format your two structs directly into variables, and then construct the expected values using that, so you're not dependent upon the implementation of #[deriving(Show)].

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah good point.

@brendanzab
Copy link
Member Author

Oh deary. impl fmt::Show for ~[clean::Argument] in rustdoc -_-#

#[cfg(not(stage0))]
impl<T: fmt::Show> fmt::Show for ~[T] {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f.buf, "{}", self.as_slice())
Copy link
Contributor

Choose a reason for hiding this comment

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

This should just be

self.as_slice().fmt(f)

@brendanzab
Copy link
Member Author

Please don't r+ just yet, gotta fix some things and re-run the tests.

@alexcrichton
Copy link
Member

Nice work! r=me with the tests fixed

@@ -296,6 +296,18 @@ fn typarams(w: &mut io::Writer,
}
}

fn arguments_to_str(args: &[clean::Argument]) -> ~str {
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be simpler if you defined

struct Arguments(~[clean::Argument]);

and implemented fmt::Show on that instead. Besides being perhaps cleaner, it also matches the previous behavior, whereas this function here will actually apply any padding/flags from the formatter to the string as a whole.

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, that's probably a better way. Thanks!

@lilyball
Copy link
Contributor

r=me if you change the arguments_to_str() as commented.

@brendanzab brendanzab deleted the show branch February 14, 2014 00:54
flip1995 pushed a commit to flip1995/rust that referenced this pull request Mar 7, 2024
Document manifest options

All built-in cargo commands have this output included when run with `cargo foo --help`. Clippy is the only exception, so I have copied the text here. As far as I can tell, the flags come from Cargo itself and not clippy, but the user almost certainly does not care about that.

```text
Manifest Options:
      --manifest-path <PATH>  Path to Cargo.toml
      --frozen                Require Cargo.lock and cache are up to date
      --locked                Require Cargo.lock is up to date
      --offline               Run without accessing the network
```

changelog: Add the standard Manifest Options section to the `--help` output
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.

4 participants