-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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 .write_str() to std::rt::io, and port to rt::io for libstd Repr and ToStr #8089
Conversation
|
||
/// Write a string representation of `self` to `w` | ||
#[inline] | ||
fn to_str_writer<W: StringWriter>(&self, w: &mut W) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unfortunate that this has the danger of someone writing impl ToStr for T {}
by accident and then being confused when their stringification crashes the program. I think there was discussion of using attributes to flag at least one of the methods in the trait as being required, but I'm not sure that went anywhere. I don't think that either of these is a good candidate for being blank by default because to_str
is natural, but slow, and to_str_writer
is efficient, but doesn't quite make sense initially.
On the other hand, I think that having this method is awesome! This may not be something that should block this initially, but others might have opinions on this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, thanks for finding that issue! Could you put a FIXME in the relevant location so when the issue is dealt with the spots in the code could also be found?
Also, I'm not sure that W: StringWriter
is the right bound here. Could W: Writer
also work? I'm not sure if the method name should change, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just make the StringWriter method mandatory, i.e. not a default method?
Found it alex, issue #7771 for marking "at least one method must be implemented". |
@@ -273,6 +274,32 @@ pub trait WriterByteConversions { | |||
fn write_i8(&mut self, n: i8); | |||
} | |||
|
|||
pub trait StringWriter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With all the new default method work, could these be implemented directly on the rt::io::Writer
trait? I figured that all of the byte conversion methods would eventually move over into that trait as well, although those working on the runtime would have a better idea of that than I would.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They could, but I want the trait separate (mentioned in a previous comment), and it sounds like a StringWriter trait will have to be re-homed to a different writer layer later, so better keep it separate. I think it's nice how the trait system makes it easy to keep parts separate.
I'm curious what new IO insiders like @brson think about this change. mostly if it's the right direction to add string methods to the writer extensions. |
My belief is that adding string encoding and decoding directly to the writers and readers is ultimately not the right approach because there may be different policies for different scenarios, e.g. regarding line terminators. My hunch is that some sort of decorator pattern is appropriate. That said I haven't put much thought into it and this does make forward progress so I'm not necessarily opposed on those grounds. |
brson, yes I think the StringWriter trait is a simple piece that can be re-homed when Text wrappers appear in the IO. |
Ok so the name for the method A precedent is best alternative name, |
@blake2-ppc How about |
So this seems like great progress, and I'm sorry I let it sit here for a few days. @blake2-ppc can you fix the merge conflicts? One thing that occurs to me is that when me redesign |
I don't know where ToStr fits into the future of |
@blake2-ppc I've given you the wrong impression. Sorry. This generally looks good to me. Can you name the method |
Consensus seems to be that both should be default methods, so scratch that. We'll just accept the iloop risk. Please rename to |
do you think the aliasing is ok for |
StringWriter implements .write_char(), .write_str(), .write_line(). Is a separate utility trait since it can be separately useful without the full Writer trait.
Add a method called .to_str_writer<W: std::rt:io::StringWriter>(w: &mut W) to ToStr. Allow passing down a string writer, that will make recursive calls to ToStr more efficient once .to_str_writer() is implemented widely. .to_str() and .to_str_writer() call each other in the default implementation. The implementer must implement at least one of these methods.
Move repr to using rt::io::mem::MemWriter instead of the old io::Writer.
Oh, I didn't realize that. It may cause some name resolution problems in the short term, and maybe having two related core methods with the same name is a bad idea in general. What's your preference? |
If ToStr is going ot be used in the future, it's not a bad idea to require the writer method; it's required to make recursive string building efficient. (But it would need a snapshot to handle the deriving). I'll push my rebase but I can't do any more work since all of the proposed changes fall in an undecided area. No use in changing it now only to have more thoughtful people roll out their fmt rewrite on top of it later. |
Port libstd Repr to use rt::io, and add a writer method to ToStr that uses rt::io::StringWriter
StringWriter implements .write_char(), .write_str(), .write_line().
Is a separate utility trait since it can be separately useful without
the full Writer trait.
Add a method called
.to_str_writer<W: std::rt:io::StringWriter>(w: &mut W)
to ToStr. Allow passing down a string writer, that will make recursive
calls to ToStr more efficient once .to_str_writer() is implemented
widely.
.to_str() and .to_str_writer() call each other in the
default implementation. The implementer must implement at least one of
these methods.
Also port std::repr to use StringWriter. It's not as pretty since we have to
replace old
@Writer
with@mut W
where W is a generic writer.