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

Figure out the default ifmt trait #8489

Closed
alexcrichton opened this issue Aug 13, 2013 · 8 comments
Closed

Figure out the default ifmt trait #8489

alexcrichton opened this issue Aug 13, 2013 · 8 comments

Comments

@alexcrichton
Copy link
Member

When using ifmt, a format argument can be simple as {} which means "the next argument" formatted as "the default". Currently the default is the ? or fmt::Poly format trait, but this could be changed. There are a few options here that we could take:

  1. Stick with Poly. This means that every type can be logged via {} by default, although some output may not be as expected. This also means it's not overridable.
  2. Define default for T: ToStr. This would basically call to_str() on the argument, and it would fail if you don't ascribe to the ToStr trait. The downside of this is that if you implement ToStr you can't override your default formatting representation.
  3. Create an entirely new fmt::Default trait. All the basic types would implement this with their respective formats. This could possibly remove the need for the s, d, u, i, c, and p traits, but that may need to be a separate issue.

I think the current default of Poly is probably a bad idea, and I'm starting to lean towards the Default trait. I think that for formatting specifiers (like {:.10s}) we would want to keep the other formats, but this means that any type could implement Default however it pleases. It also means that 99% of the formats done today could just be a dead-simple {} (although that's just a guess).

One note on performance, anything implementing the Default may not have to worry about format specifiers/flags/etc because the format specifier grammar wouldn't allow specifying these flags without specifying a format string (like s or d), and there's no reason that a format string for Default has to be defined... That's kinda a side note though.

Nominating for the backwards compatibility milestone as well (especially because the default is ? today.

@bluss
Copy link
Member

bluss commented Aug 13, 2013

ToStr cannot stay as it is. By nature (deriving) it is called recursively and allocates for each call; it needs a writer-based replacement and the String or Default formatter might be that.

@brson
Copy link
Contributor

brson commented Aug 13, 2013

cc #8089

@huonw
Copy link
Member

huonw commented Aug 15, 2013

For {}, I really think it shouldn't be ? (that can be {?}), but I don't see a reason to introduce yet another trait for it when we can just use ToStr directly. I can't think of a type where it makes sense for the ToStr impl to be different to the (proposed) Default impl. If someone wants something else, e.g. something like Python's repr, then they can use an entirely different trait (std::repr::Repr for repr, possibly with a {r} flag, like pythons %r), or just have a custom method that gets called: fmt!("{}", my_type.my_to_str()).

(On this note, it be neat if we could somehow allow fmt! to pass the writer into its args, e.g. fmt!("{}", my_type.write_out($writer)) where write_out(&self, &mut Writer).)

It might also be a nice simplification to remove all of s, d, u etc, since they do nothing more than what {} would do (even if {} just calls ToStr); although this might mean that people accidentally print something they don't want to.

@blake2-ppc: it is easy to update deriving(ToStr) to a writer-based one (especially if it was done in stages where write_str(&self, &mut Writer) and to_str(&self) -> ~str are defined as default methods in terms of each other, so it didn't have to be updated immediately), even with recursive calls.

@graydon
Copy link
Contributor

graydon commented Aug 15, 2013

I have no strong opinion on this, aside from "{} should do something most people want to see". I suspect most of the corner cases can be dealt with one way or another by providing slightly more details at the call site, this is just about the laziest-possible form.

@alexcrichton
Copy link
Member Author

From what I've been talking about with @huonw and @blake2-ppc we're all in agreement that ? is a pretty bad default. Of the two leftover, we have these pros/cons:

  • Using a separate Default trait means that any type can implement it any way, and it can be optimized per type. Sadly, this means that it is not defined for all types out-of-the-box
  • Defining {} for all types implementing ToStr would currently be slow (an extra allocation per argument), and in the future it could be faster (with a to_writer method that wrote directly into an io::Writer). The good part about this is that it's defined for almost all types out-of-the-box. The bad part about this is that even with a to_writer method there still could be unnecessary allocations. Primarily, if a width formatting flag is provided an extra allocation would have to be made. Additionally, {:10.10f} would be very different from {:10.10} applied to a float because there's no longer an understanding of what the precision is.

I'm personally still leaning towards Default, but now for the main reason that adding or removing the format specifier can result in different outputs. In the case of Default, the formats would be exactly the same (the type would imply which formatter to delegate to).

In the far future, python has formats like {:%Y-%M-%D %h:%m:%s} which could format a date. In theory the format syntax could interpret this and pass this slice of a "format argument" on to a formatter to use. If "no format specified" implied the ToStr result, then it's not really that customizable and this behavior wouldn't be very possible without having another format. It would be kinda slick if we could do this, but currently I'm not sure how to disambiguate what comes after the colon for this, so it's kinda a separate issue.

@huonw, @blake2-ppc, with this in mind, do you guys still think that ToStr is the way to go?

@alexcrichton
Copy link
Member Author

Right now an implementation of Default would look like alexcrichton@802de72, although that's certainly not set in stone, I just had some extra time this afternoon

@huonw
Copy link
Member

huonw commented Aug 15, 2013

I'd (embarrassingly) forgotten about the precision & width arguments, so I agree that we need a separate trait for {} formatting, if it is going to be consistent and efficient.

bors added a commit that referenced this issue Aug 19, 2013
See discussion in #8489, but this selects option 3 by adding a `Default` trait to be implemented by various basic types.

Once this makes it into a snapshot I think it's about time to start overhauling all current use-cases of `fmt!` to move towards `ifmt!`. The goal is to replace `%X` with `{}` in 90% of situations, and this commit should enable that.
@alexcrichton
Copy link
Member Author

Closed by #8564

flip1995 pushed a commit to flip1995/rust that referenced this issue Mar 14, 2022
Add `unnecessary_find_map` lint

This PR adds an `unnecessary_find_map` lint. It is essentially just a minor enhancement of `unnecessary_filter_map`.

Closes rust-lang#8467

changelog: New lint `unnecessary_find_map`
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

No branches or pull requests

5 participants