code review 162041: gofmt: use os.Stdin instead of opening /dev/stdin

gofmt: use os.Stdin instead of opening /dev/stdin

Opening /dev/stdin can sometimes fail. For example, in the acme editor,
executing "Edit ,|gofmt" fails with:

	open /dev/stdin: no such device or address

Executing "Edit ,|ls -l /dev/stdin /proc/self/fd/0" gives:

	lrwxrwxrwx 1 root root  15 2009-09-07 02:17 /dev/stdin -> /proc/self/fd/0
	lrwx------ 1 fhs  users 64 2009-11-26 22:05 /proc/self/fd/0 -> socket:[5528230]


(This is my first change, and I've signed the individual contributor license agreement.)

[fhs, 2009-11-27 03:28:01]

Hello rsc, gri (cc: golang-dev@googlegroups.com),

I'd like you to review the following change.

[rsc, 2009-11-29 23:24:06]

Arguably this is working around a bug in Linux, but
it's cleaner to use os.Stdin instead of depending on
being able to open stdin anyway.

Leaving for gri to review; I'll take care of the CONTRIBUTORS
and AUTHORS changes.

Thanks.

On Thu, Nov 26, 2009 at 19:28,  <fshahriar@gmail.com> wrote:
> Reviewers: rsc, gri,
>
> Message:
> Hello rsc, gri (cc: golang-dev@googlegroups.com),
>
> I'd like you to review the following change.
>
>
> Description:
> gofmt: use os.Stdin instead of opening /dev/stdin
>
> Opening /dev/stdin can sometimes fail. For example, in the acme editor,
> executing "Edit ,|gofmt" fails with:
>
>        open /dev/stdin: no such device or address
>
> Executing "Edit ,|ls -l /dev/stdin /proc/self/fd/0" gives:
>
>        lrwxrwxrwx 1 root root  15 2009-09-07 02:17 /dev/stdin ->
> /proc/self/fd/0
>        lrwx------ 1 fhs  users 64 2009-11-26 22:05 /proc/self/fd/0 ->
> socket:[5528230]
>
>
> (This is my first change, and I've signed the individual contributor
> license agreement.)
>
> Please review this at http://codereview.appspot.com/162041
>
> Affected files:
>  M src/cmd/gofmt/gofmt.go
>
>
> Index: src/cmd/gofmt/gofmt.go
> ===================================================================
> --- a/src/cmd/gofmt/gofmt.go
> +++ b/src/cmd/gofmt/gofmt.go
> @@ -79,11 +79,12 @@
>  }
>
>
> -func processFile(filename string) os.Error {
> -       src, err := io.ReadFile(filename);
> +func processFile(f *os.File) os.Error {
> +       src, err := io.ReadAll(f);
>        if err != nil {
>                return err
>        }
> +       filename := f.Name();
>
>        file, err := parser.ParseFile(filename, src, parserMode());
>        if err != nil {
> @@ -131,7 +132,13 @@
>  func (v fileVisitor) VisitFile(path string, d *os.Dir) {
>        if isGoFile(d) {
>                v <- nil;       // synchronize error handler
> -               if err := processFile(path); err != nil {
> +               file, err := os.Open(path, os.O_RDONLY, 0);
> +               if err != nil {
> +                       v <- err;
> +                       return;
> +               }
> +               defer file.Close();
> +               if err := processFile(file); err != nil {
>                        v <- err
>                }
>        }
> @@ -165,7 +172,7 @@
>        initRewrite();
>
>        if flag.NArg() == 0 {
> -               if err := processFile("/dev/stdin"); err != nil {
> +               if err := processFile(os.Stdin); err != nil {
>                        report(err)
>                }
>        }
> @@ -176,9 +183,15 @@
>                case err != nil:
>                        report(err)
>                case dir.IsRegular():
> -                       if err := processFile(path); err != nil {
> +                       file, err := os.Open(path, os.O_RDONLY, 0);
> +                       if err != nil {
> +                               report(err);
> +                               continue;
> +                       }
> +                       if err := processFile(file); err != nil {
>                                report(err)
>                        }
> +                       file.Close();
>                case dir.IsDirectory():
>                        walkDir(path)
>                }
>
>
>

[gri, 2009-11-30 19:09:13]

http://codereview.appspot.com/162041/diff/1001/4
File src/cmd/gofmt/gofmt.go (right):

http://codereview.appspot.com/162041/diff/1001/4#newcode89
src/cmd/gofmt/gofmt.go:89: file, err := parser.ParseFile(filename, src,
parserMode());
s/filename/f.Name()/ and get rid of the extra variable declaration on line 87

http://codereview.appspot.com/162041/diff/1001/4#newcode135
src/cmd/gofmt/gofmt.go:135: file, err := os.Open(path, os.O_RDONLY, 0);
This code and the code in walkDir are basically the same. Please factor by
having a helper function, say: processFileByName(filename string) or.Error {...}
that takes the place of what processFile did originally. It can simply call (the
new version of) processFile.

http://codereview.appspot.com/162041/diff/1001/4#newcode186
src/cmd/gofmt/gofmt.go:186: file, err := os.Open(path, os.O_RDONLY, 0);
see comment above

[fhs, 2009-11-30 23:58:56]

PTAL

On 2009/11/30 19:09:13, gri wrote:
> http://codereview.appspot.com/162041/diff/1001/4
> File src/cmd/gofmt/gofmt.go (right):
> 
> http://codereview.appspot.com/162041/diff/1001/4#newcode89
> src/cmd/gofmt/gofmt.go:89: file, err := parser.ParseFile(filename, src,
> parserMode());
> s/filename/f.Name()/ and get rid of the extra variable declaration on line 87

Done.

> 
> http://codereview.appspot.com/162041/diff/1001/4#newcode135
> src/cmd/gofmt/gofmt.go:135: file, err := os.Open(path, os.O_RDONLY, 0);
> This code and the code in walkDir are basically the same. Please factor by
> having a helper function, say: processFileByName(filename string) or.Error
{...}
> that takes the place of what processFile did originally. It can simply call
(the
> new version of) processFile.

Done.

> 
> http://codereview.appspot.com/162041/diff/1001/4#newcode186
> src/cmd/gofmt/gofmt.go:186: file, err := os.Open(path, os.O_RDONLY, 0);
> see comment above

Done.

[gri, 2009-12-01 17:31:29]

LGTM

Thank you.

[gri, 2009-12-01 17:32:24]

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

gofmt: use os.Stdin instead of opening /dev/stdin

Opening /dev/stdin can sometimes fail. For example, in the acme editor,
executing "Edit ,|gofmt" fails with:

	open /dev/stdin: no such device or address

Executing "Edit ,|ls -l /dev/stdin /proc/self/fd/0" gives:

	lrwxrwxrwx 1 root root  15 2009-09-07 02:17 /dev/stdin -> /proc/self/fd/0
	lrwx------ 1 fhs  users 64 2009-11-26 22:05 /proc/self/fd/0 -> socket:[5528230]


(This is my first change, and I've signed the individual contributor license
agreement.)

R=rsc, gri
CC=golang-dev
http://codereview.appspot.com/162041

Committer: Robert Griesemer <gri@golang.org>