Issue 87780046: code review 87780046: io: document that a Writer must not write to p (Closed)
Descriptionio: document that a Writer must not write to p
Per golang-nuts question. Writing to p breaks
other writers (e.g. io.MultiWriter).
Make this explicit.
Patch Set 1 #Patch Set 2 : diff -r 8e5787506b59 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 8e5787506b59 https://go.googlecode.com/hg/ #
Total comments: 1
Patch Set 4 : diff -r 8e5787506b59 https://go.googlecode.com/hg/ #
Total comments: 3
Patch Set 5 : diff -r 8e5787506b59 https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 6 : diff -r b7951abc878e https://go.googlecode.com/hg/ #MessagesTotal messages: 19
Hello r@golang.org, rsc@golang.org, gri@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
https://codereview.appspot.com/87780046/diff/40001/src/pkg/io/io.go File src/pkg/io/io.go (right): https://codereview.appspot.com/87780046/diff/40001/src/pkg/io/io.go#newcode77 src/pkg/io/io.go:77: // Write must not write to p. Write must not modify the contents of the slice p.
Sign in to reply to this message.
On 2014/04/15 21:50:23, bradfitz wrote: > Hello mailto:r@golang.org, mailto:rsc@golang.org, mailto:gri@golang.org (cc: > mailto:golang-codereviews@googlegroups.com), > > I'd like you to review this change to > https://go.googlecode.com/hg/ Quick note: I think it might be clearer to say "must not modify p" rather than "must not write to p".
Sign in to reply to this message.
I use the word "write" instead of "modify" because modify implies that it's okay to write the same value back to it. The race detector and memory model would disagree.
Sign in to reply to this message.
Hello r@golang.org, rsc@golang.org, gri@golang.org, joshlf13@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
Rob? On Tue, Apr 15, 2014 at 3:10 PM, <gri@golang.org> wrote: > LGTM > > https://codereview.appspot.com/87780046/ >
Sign in to reply to this message.
https://codereview.appspot.com/87780046/diff/60001/src/pkg/io/io.go File src/pkg/io/io.go (right): https://codereview.appspot.com/87780046/diff/60001/src/pkg/io/io.go#newcode77 src/pkg/io/io.go:77: // Write must not write to the slice p. I'm sorry but "Write must not write..." is odd and confusing phrasing. Plus it can modify p (the variable), just not the contents. That's why I made my suggestion, which I still prefer: "Write must not modify the contents of p." Also, it's the implementation we're talking about, not the interface, and you called it "it" already,so how about: It must return a non-nil error if it returns n < len(p). Also, it must not modify the contents of p.
Sign in to reply to this message.
https://codereview.appspot.com/87780046/diff/60001/src/pkg/io/io.go File src/pkg/io/io.go (right): https://codereview.appspot.com/87780046/diff/60001/src/pkg/io/io.go#newcode77 src/pkg/io/io.go:77: // Write must not write to the slice p. On 2014/04/15 22:49:58, r wrote: > I'm sorry but "Write must not write..." is odd and confusing phrasing. Plus it > can modify p (the variable), just not the contents. That's why I made my > suggestion, which I still prefer: "Write must not modify the contents of p." > > Also, it's the implementation we're talking about, not the interface, and you > called it "it" already,so how about: > > It must return a non-nil error if it returns n < len(p). > Also, it must not modify the contents of p. I still object. There's a difference between leaving the buffer how you found it when you return from Write vs data races in the presence of many goroutines reading from the same buffer. In fact, the very next question on the mailing list was _just that_. We need to be clear here that assigning/writing (pick a word) to the contents of the p is not allowed.
Sign in to reply to this message.
https://codereview.appspot.com/87780046/diff/60001/src/pkg/io/io.go File src/pkg/io/io.go (right): https://codereview.appspot.com/87780046/diff/60001/src/pkg/io/io.go#newcode77 src/pkg/io/io.go:77: // Write must not write to the slice p. Sigh. Well, at least if we use 'it' we don't have "Write must not write". It must not write to the contents of the slice p.
Sign in to reply to this message.
I don't believe we have to answer every golang-nuts question in the docs. I don't believe "modify" allows "change and change back". If you have to write something, I agree with Rob: use something like "Write must not modify the content of the slice." But probably we can just not say anything.
Sign in to reply to this message.
I want docs. Users want docs. On Tue, Apr 15, 2014 at 4:03 PM, Russ Cox <rsc@golang.org> wrote: > I don't believe we have to answer every golang-nuts question in the docs. > I don't believe "modify" allows "change and change back". > If you have to write something, I agree with Rob: use something like > "Write must not modify the content of the slice." > But probably we can just not say anything. > >
Sign in to reply to this message.
Hello r@golang.org, rsc@golang.org, gri@golang.org, joshlf13@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM by which i mean, i hate this and would prefer we said nothing and i like my original wording much MUCH better but please god make it stop
Sign in to reply to this message.
THIS IS FOR ROT13??? reversing my LGTM On Tue, Apr 15, 2014 at 4:08 PM, <bradfitz@golang.org> wrote: > Hello r@golang.org, rsc@golang.org, gri@golang.org, joshlf13@gmail.com > (cc: golang-codereviews@googlegroups.com), > > Please take another look. > > > https://codereview.appspot.com/87780046/
Sign in to reply to this message.
Leaving for rsc.
Sign in to reply to this message.
It was from our tour, was it not? It's a natural question to ask. I'm glad he asked it, because writing to the buffer corrupts io.MultiWriter users. On Tue, Apr 15, 2014 at 4:10 PM, Rob Pike <r@golang.org> wrote: > THIS IS FOR ROT13??? > > reversing my LGTM > > On Tue, Apr 15, 2014 at 4:08 PM, <bradfitz@golang.org> wrote: > > Hello r@golang.org, rsc@golang.org, gri@golang.org, joshlf13@gmail.com > > (cc: golang-codereviews@googlegroups.com), > > > > Please take another look. > > > > > > https://codereview.appspot.com/87780046/ >
Sign in to reply to this message.
LGTM with edits below https://codereview.appspot.com/87780046/diff/10002/src/pkg/io/io.go File src/pkg/io/io.go (right): https://codereview.appspot.com/87780046/diff/10002/src/pkg/io/io.go#newcode76 src/pkg/io/io.go:76: // It must return a non-nil error if it returns n < len(p). s/It/Write/ (there are a lot of nouns in the previous sentence.) https://codereview.appspot.com/87780046/diff/10002/src/pkg/io/io.go#newcode77 src/pkg/io/io.go:77: // Also, it must not write to the contents of the slice p. // Write must not modify the slice data, even temporarily.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=c7065d3ad91a *** io: document that a Writer must not write to p Per golang-nuts question. Writing to p breaks other writers (e.g. io.MultiWriter). Make this explicit. LGTM=gri, r, rsc R=r, rsc, gri, joshlf13 CC=golang-codereviews https://codereview.appspot.com/87780046
Sign in to reply to this message.
|
