Add the formatter_len_hint method to Show and use it in to_string
#16544
Conversation
| @@ -148,113 +171,159 @@ impl<'a> Show for Arguments<'a> { | |||
| pub trait Show { | |||
| /// Formats the value using the given formatter. | |||
| fn fmt(&self, &mut Formatter) -> Result; | |||
|
|
|||
| /// Returns an upper bound on the size of the formatted string. | |||
There was a problem hiding this comment.
I think this should actually be a lower bound, or just a loosely defined "conservative estimate" (since a true upper bound can be much larger than the real value, leading to the same overallocation problems). In this case, returning 0 by default makes sense.
|
In general, can you provide data to support a change such as this? The issue only claims that it may be an issue, I don't see any concrete numbers one way or the other. I'd like to see some analysis which compares the heap usage before and afterwards of projects like rustc (and perhaps servo) to see if a change such as this reaps the theoretical benefits. In the past the code size generated by |
(This can be addressed by storing a single pointer to a struct/tuple/vtable containing the two function pointers.) |
| let log2base = 1 << $log2log2base; | ||
|
|
||
| // Get the number of digits in the target base. | ||
| let binary_digits = width - (num | log2base).leading_zeros() as uint; |
There was a problem hiding this comment.
To adjust for the fact that zero has one digit, so that
- there are at most
width - log2base - 1leading zeros - binary_digits is at least
log2base + 1 binary_digits / log2baseis never zero
|
Note also that instead of changing the definition of |
|
@pnkfelix, I agree. The change to I've just made these measurements. |
|
Here's why I forgot about it before. |
pczarn
changed the title
size_hint method to Show as a default methodsize_hint method to Show as a default method and use it in to_string
pczarn
changed the title
size_hint method to Show as a default method and use it in to_stringsize_hint method to Show as a default method and use it in to_string
|
pub trait Show {
// ...
fn size_hint(val: &Self) -> Option<uint> { None }
}thus turning usage into e.g. let mut output = io::MemWriter::with_capacity(Show::size_hint(self).unwrap_or(128));Although I am unsure how UFCS affects static trait methods. |
|
Edit: What am I thinking, third-party types can't implement |
|
This method has to work for many types. I agree that a method used once internally shouldn't have a generic name. Well, at least iterators don't implement How about changing it to |
|
Renamed to |
pczarn
force-pushed
the
formatting_size_hint
branch
from
91ccbd5 to
fbb2981
Compare
|
Updated with a simplified implementation for floats. Added two tests. |
|
This is good to go, right? I have a crude memory chart from stage1. Before:magenta, after:blue, the peak is smaller by 4MB. |
pczarn
force-pushed
the
formatting_size_hint
branch
from
6cfef4e to
0e2ba80
Compare
pczarn
changed the title
size_hint method to Show as a default method and use it in to_stringformatter_len_hint method to Show and use it in to_string
Implements `formatter_len_hint` for several simple types. Length of the formatted string is approximated for floats and integers.
pczarn
force-pushed
the
formatting_size_hint
branch
from
0e2ba80 to
fbc04e6
Compare
|
Note that there has been talk of separating Committing to this style of API is taking us pretty hard down the road of using I'm curious, @aturon, do you have an opinion on this? This is something that can always be added backwards-compatibly later on, and I'd almost rather consider the stabilization of |
|
I believe the win is bigger, but most of it is already in (using |
|
@pczarn, thanks for taking this on! I agree with @alexcrichton that, while this seems like a reasonable extension, it seems wise to resolve the basic question about the role of I'll try to get a draft together in the next day or two, and ping you for feedback before posting it. |
|
Closing to help clear out the queue, @aturon do you have an update on the RFC you were planning to write? |
|
|
…ykril Add completions to show only traits in trait `impl` statement This is prerequisite PR for adding the assist mentioned in rust-lang#12500 P.S: If wanted, I will add the implementation of the assist in this PR as well.
I'm trying to balance potential code bloat and precision of the upper bound.
size_hintdefault to 0 instead of None?size_hintfor floats? I've yet to think about it.size_hintalso that useful for the general case offormat!, as implemented in this PR, not onlyto_string?UFCS would make this addition prettier.Examples
Fixes #16415