code review 6558046: pkg/go/ast: Avoid doing zero-length writes to the fd.

pkg/go/ast: Avoid doing zero-length writes to the fd.

After each line, ast.Print would do a zero-length write,
which would hit the boundary condition on Plan 9 when
reading over pipes (since message boundaries are
preserved). This change makes sure we only do positive-
length writes.

[akumar, 2012-09-23 21:47:50]

Hello rsc@golang.org, rminnich@gmail.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[dave_cheney.net, 2012-09-23 21:50:30]

This CL sounds reasonable, but wouldn't output be buffered by something
that is coalescing writes?
On 24 Sep 2012 07:47, <seed@mail.nanosouffle.net> wrote:

> Reviewers: rsc, rminnich,
>
> Message:
> Hello rsc@golang.org, rminnich@gmail.com (cc:
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> pkg/go/ast: Avoid doing zero-length writes to the fd.
>
> After each line, ast.Print would do a zero-length write,
> which would hit the boundary condition on Plan 9 when
> reading over pipes (since message boundaries are
> preserved). This change makes sure we only do positive-
> length writes.
>
> Please review this at
http://codereview.appspot.com/**6558046/<http://codereview.appspot.com/6558046/>
>
> Affected files:
>   M src/pkg/go/ast/print.go
>
>
> Index: src/pkg/go/ast/print.go
> ==============================**==============================**=======
> --- a/src/pkg/go/ast/print.go
> +++ b/src/pkg/go/ast/print.go
> @@ -108,8 +108,11 @@
>                 }
>                 p.last = b
>         }
> -       m, err = p.output.Write(data[n:])
> -       n += m
> +       // Avoid doing zero-length writes.
> +       if len(data)-n > 0 {
> +               m, err = p.output.Write(data[n:])
> +               n += m
> +       }
>         return
>  }
>
>
>
>

[r, 2012-09-23 21:56:01]

http://codereview.appspot.com/6558046/diff/4001/src/pkg/go/ast/print.go
File src/pkg/go/ast/print.go (right):

http://codereview.appspot.com/6558046/diff/4001/src/pkg/go/ast/print.go#newco...
src/pkg/go/ast/print.go:112: if len(data)-n > 0 {
if len(data) > n

[r, 2012-09-23 21:57:34]

http://codereview.appspot.com/6558046/diff/4001/src/pkg/go/ast/print.go
File src/pkg/go/ast/print.go (right):

http://codereview.appspot.com/6558046/diff/4001/src/pkg/go/ast/print.go#newco...
src/pkg/go/ast/print.go:111: // Avoid doing zero-length writes.
delete this comment. it's not an accurate summary of the code that follows -
it's writing, not avoiding - and the avoidance of zero-length writes is
self-evident.

[akumar, 2012-09-23 22:00:41]

PTAL.

http://codereview.appspot.com/6558046/diff/4001/src/pkg/go/ast/print.go
File src/pkg/go/ast/print.go (right):

http://codereview.appspot.com/6558046/diff/4001/src/pkg/go/ast/print.go#newco...
src/pkg/go/ast/print.go:111: // Avoid doing zero-length writes.
On 2012/09/23 21:57:34, r wrote:
> delete this comment. it's not an accurate summary of the code that follows -
> it's writing, not avoiding - and the avoidance of zero-length writes is
> self-evident.

Done.

http://codereview.appspot.com/6558046/diff/4001/src/pkg/go/ast/print.go#newco...
src/pkg/go/ast/print.go:112: if len(data)-n > 0 {
On 2012/09/23 21:56:01, r wrote:
> if len(data) > n

Done.

[r, 2012-09-23 22:02:58]

http://codereview.appspot.com/6558046/diff/9001/src/pkg/go/ast/print.go
File src/pkg/go/ast/print.go (right):

http://codereview.appspot.com/6558046/diff/9001/src/pkg/go/ast/print.go#newco...
src/pkg/go/ast/print.go:111: if len(data) > n {
it would be simpler to put
if len(data) == 0 {
  return
}
at the top

[akumar, 2012-09-23 22:21:46]

On 2012/09/23 22:02:58, r wrote:
> http://codereview.appspot.com/6558046/diff/9001/src/pkg/go/ast/print.go
> File src/pkg/go/ast/print.go (right):
> 
>
http://codereview.appspot.com/6558046/diff/9001/src/pkg/go/ast/print.go#newco...
> src/pkg/go/ast/print.go:111: if len(data) > n {
> it would be simpler to put
> if len(data) == 0 {
>   return
> }
> at the top

It's not necessary that len(data) = 0; we can have len(data) = n > 0
when the data is being pushed out at the end of that loop, as is
the case when we do p.printf("\n") I believe.

[r, 2012-09-23 22:27:19]

LGTM

[r, 2012-09-23 22:30:54]

*** Submitted as http://code.google.com/p/go/source/detail?r=6fdbfbfa8813 ***

pkg/go/ast: Avoid doing zero-length writes to the fd.

After each line, ast.Print would do a zero-length write,
which would hit the boundary condition on Plan 9 when
reading over pipes (since message boundaries are
preserved). This change makes sure we only do positive-
length writes.

R=rsc, rminnich, dave, r
CC=golang-dev
http://codereview.appspot.com/6558046

Committer: Rob Pike <r@golang.org>

[bradfitz, 2012-09-23 22:58:38]

I assume you'll also fix the plan9 pkg os code to just work with 0-length
writes? That seems like the real problem.
On Sep 23, 2012 2:47 PM, <seed@mail.nanosouffle.net> wrote:

> Reviewers: rsc, rminnich,
>
> Message:
> Hello rsc@golang.org, rminnich@gmail.com (cc:
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> pkg/go/ast: Avoid doing zero-length writes to the fd.
>
> After each line, ast.Print would do a zero-length write,
> which would hit the boundary condition on Plan 9 when
> reading over pipes (since message boundaries are
> preserved). This change makes sure we only do positive-
> length writes.
>
> Please review this at
http://codereview.appspot.com/**6558046/<http://codereview.appspot.com/6558046/>
>
> Affected files:
>   M src/pkg/go/ast/print.go
>
>
> Index: src/pkg/go/ast/print.go
> ==============================**==============================**=======
> --- a/src/pkg/go/ast/print.go
> +++ b/src/pkg/go/ast/print.go
> @@ -108,8 +108,11 @@
>                 }
>                 p.last = b
>         }
> -       m, err = p.output.Write(data[n:])
> -       n += m
> +       // Avoid doing zero-length writes.
> +       if len(data)-n > 0 {
> +               m, err = p.output.Write(data[n:])
> +               n += m
> +       }
>         return
>  }
>
>
>
>

[rsc, 2012-09-24 03:13:35]

On Sun, Sep 23, 2012 at 6:58 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> I assume you'll also fix the plan9 pkg os code to just work with 0-length
> writes? That seems like the real problem.

Trust me, you don't want to start this discussion.

Russ