code review 179074: A stringio for Go. Seems making sense to have it includ... (issue179074)
i3dm...@gmail.com
Message:
Hello golan...@googlegroups.com (cc: golan...@googlegroups.com),
I'd like you to review the following change.
Description:
A stringio for Go. Seems making sense to have it included in the io
package. Please take a look.
Please review this at http://codereview.appspot.com/179074
Affected files:
M src/pkg/io/Makefile
A src/pkg/io/stringio.go
A src/pkg/io/stringio_test.go
r...@golang.org
This doesn't belong in io, which should have as
little code as possible to avoid dependencies.
In particular, types that exist to implement io
interfaces belong in other packages. This one
belongs in bytes, next to bytes.Buffer.
I suggest that you read through the bytes.Buffer
implementation and make sure you understand
every line before proceeding. You lifted its resize
but used it in a way that doesn't make sense with
the rest of the code. The Buffer implementation
is a good demonstration of how to use slices; it will
repay careful study. In particular pay attention to
the difference between len(buf) (the number of
valid data bytes) and cap(buf) (the total amount of
storage) and how the buffer uses them to good effect.
I'm torn about whether this should be separate
from bytes.Buffer, and if so, what it's name should be.
One option is to expand bytes.Buffer, but
we've avoided the assumption that a bytes.Buffer
is seekable, so that a long-lived stream can be
implemented efficiently as long as the reader and
writer stay near each other. In fact we just put
that optimization in yesterday. Also, Buffer has
two different offsets, one for reading and one for
writing, so Seek doesn't make sense.
Assuming Buffer stays as it is, then we do need
a seekable version, but I don't have a good name
for it. Maybe just bytes.File. That's probably fine
for now: put it in bytes/file.go and address the
comments below (you might want to start with
buffer.go and adapt/merge in your new methods).
There are a bunch of specific comments below.
http://codereview.appspot.com/179074/diff/1013/1015
File src/pkg/io/stringio.go (right):
http://codereview.appspot.com/179074/diff/1013/1015#newcode14
src/pkg/io/stringio.go:14: var OSError os.Error = os.NewError("I/O
operation on closed file")
OSError is a pretty vague name for this error.
It's such an uncommon case that os.EINVAL is probably fine.
Also, Close might as well be a no-op.
http://codereview.appspot.com/179074/diff/1013/1015#newcode16
src/pkg/io/stringio.go:16: // A StringIO object is similar to a File
object.
I know that this name is from Java (or Python, or Ruby, or ...)
but it's a terrible name: this type has little to do with strings
and is already in package io.
You note that it's very closely related to bytes.Buffer, so it
probably belongs in package bytes. In fact it can probably
reuse a lot of the bytes.Buffer implementation.
http://codereview.appspot.com/179074/diff/1013/1015#newcode17
src/pkg/io/stringio.go:17: // It mimics all File I/O operations by
implementing the
Because Go has such good support for fine-grained interfaces,
it's not terribly interesting to mimic all the os.File methods.
Functions that take an interface will say what methods they
need, and if one of those methods doesn't apply here, it's
probably not useful to pass one of these to that function.
For example, a function that expects an Fd() method
probably needs a real fd. Conversely, functions that don't
need Fd methods won't ask for it. So there's no point in
having a dummy Fd method here.
On the other hand, it's definitely interesting to implement the
basic io interface methods, like ReadAt, Seek, WriteAt, etc.
http://codereview.appspot.com/179074/diff/1013/1015#newcode43
src/pkg/io/stringio.go:43: sio.last = 0
Don't need last here; use len(buf) instead.
http://codereview.appspot.com/179074/diff/1013/1015#newcode53
src/pkg/io/stringio.go:53: func (s *StringIO) GoString() string { return
s.name }
I don't know why you've chosen to implement GoString,
but it's supposed to return Go syntax. I don't think it
makes sense to implement here anyway.
http://codereview.appspot.com/179074/diff/1013/1015#newcode58
src/pkg/io/stringio.go:58: func (s *StringIO) String() string {
It seems like this would be UnreadString()
http://codereview.appspot.com/179074/diff/1013/1015#newcode59
src/pkg/io/stringio.go:59: if s.isClosed() {
I don't see the point of all the isClosed checks.
http://codereview.appspot.com/179074/diff/1013/1015#newcode66
src/pkg/io/stringio.go:66: func (s *StringIO) GetValueString() string {
and this would be String().
In general, any time you see the word Get in a method,
something has gone wrong. The fact that the method
returns something should be signal enough that it gets.
This is why, for example, bytes.Buffer has methods named
Bytes and String, not GetBytes and GetString.
http://codereview.appspot.com/179074/diff/1013/1015#newcode74
src/pkg/io/stringio.go:74: func (s *StringIO) GetValueBytes() []byte {
Similarly, should be Bytes.
http://codereview.appspot.com/179074/diff/1013/1015#newcode90
src/pkg/io/stringio.go:90: if s.isClosed() != true {
It is more conventional to write !b than b != true.
http://codereview.appspot.com/179074/diff/1013/1015#newcode105
src/pkg/io/stringio.go:105: int64_O := int64(0)
I don't understand why this uses a letter O instead of a zero.
But more importantly, I don't understand why it exists at all:
you can just use 0.
http://codereview.appspot.com/179074/diff/1013/1015#newcode124
src/pkg/io/stringio.go:124: if ret > length {
It is more convention to give an error if you're
not going to allow the seek. Silently returning a
different offset is bound to break callers, which
assume that success means it seeked to the
desired location.
http://codereview.appspot.com/179074/diff/1013/1015#newcode146
src/pkg/io/stringio.go:146: s.setPos(offset)
This is not the semantics of ReadAt.
One of the reasons ReadAt exists is so that
multiple goroutines can use it simultaneously
without fighting over an offset. A better
implementation is to pass the offset to s.readBytes.
http://codereview.appspot.com/179074/diff/1013/1015#newcode163
src/pkg/io/stringio.go:163: s.setPos(offset)
Same comment as for ReadAt.
http://codereview.appspot.com/179074/diff/1013/1015#newcode168
src/pkg/io/stringio.go:168: b := syscall.StringByteSlice(str)
This is one reason not to put the code in package io.
Then you can use strings.Bytes.
http://codereview.appspot.com/179074/diff/1013/1015#newcode173
src/pkg/io/stringio.go:173: // private methods
comment is unnecessary
http://codereview.appspot.com/179074/diff/1013/1015#newcode203
src/pkg/io/stringio.go:203: pos, int64_O, length := int64(s.pos),
int64(0), int64(len(s.buf))
int64_O is unnecessary; use 0
http://codereview.appspot.com/179074/diff/1013/1015#newcode216
src/pkg/io/stringio.go:216: func (s *StringIO) isClosed() bool { return
s.isclosed == true }
This is a typical Javaism.
There's no need for it: just use s.isclosed
directly instead of calling s.isClosed.
It's shorter and faster.
Also, the convention is to write b instead of b == true.
http://codereview.appspot.com/179074/diff/1013/1015#newcode222
src/pkg/io/stringio.go:222: copy(buf, s.buf[0:])
s.buf[0:] is just s.buf.
This may be stolen from bytes.Buffer, but it doesn't
apply here unless you manage the length and cap the
same way (which I suggest doing).
http://codereview.appspot.com/179074
i3dm...@gmail.com
Thanks for the comments! Since I've also post this idea into the group
channel, Rob had replied with some of his comments too, but looks like
you guys have a different opinion on whether random access should be
supported in Buffer. Can you take a look Issue 440 and maybe talk with
him and see if you guys can get a consensus on that?
Also, he seems recommending to have these extra IO functions implemented
directly into Buffer module instead of creating a new one, though
personally I don't have a strong feeling that a Buffer should carry so
much of a File semantics but I can also see the his point (and yours
too) because there are quite a bit of code overlap there. I did
originally thought to implement File related methods in Buffer, but
since I don't know whether I should touch it too much especially what I
do will bring some new meanings to Buffer, I chose to just write a new
one. I think the central point is should Buffer implements Seeker and
Closer?
i3dm...@gmail.com
Buffer?
Thanks,
Jim
Russ Cox
> Any updates Russ? Should it be a separate module
no updates. i'm about to disappear for a week or so,
so this might have to wait until the new year.
i'm curious: what do you want to use this for?
russ
i3dm...@gmail.com
In general, this module would most likely be used in testing and small
in-memory storage that do not require to touch the underly filesystem.
Its quite common to use it to in testing to factor out the filesystem
dependency by faking the file object that the caller requires with such
a file like object that only operates against buffers.
I think it would be nice if we can implement file operations on top of
Buffer so that it can be used to replace a real file object but if the
semantics of file does not seem to apply well with Buffer, than we may
have to just create a different module for it.
Another thought is that although Go creates a wonderful way to implement
interfaces, say, a client developer can just write up a file like struct
by implementing the interfaces that File has, it still requires them to
do that every time, it seems making sense to create a common module in
the standard library to offer such functionality.
Thanks,
Jim
On 2009/12/21 15:21:40, rsc wrote:
> On Sun, Dec 20, 2009 at 16:06, <mailto:i3dm...@gmail.com> wrote:
> > Any updates Russ? Should it be a separate module
> no updates. i'm about to disappear for a week or so,
> so this might have to wait until the new year.
> i'm curious: what do you want to use this for?
> russ