code review 4259065: gotype: commandline tool to typecheck go programs

gofront: commandline tool to typecheck go programs

[gri, 2011-03-10 07:32:06]

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

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

[gri, 2011-03-10 18:33:58]

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

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

[r, 2011-03-10 18:42:43]

seems reasonable as code but what is the motivation?

http://codereview.appspot.com/4259065/diff/3006/src/cmd/gocheck/doc.go
File src/cmd/gocheck/doc.go (right):

http://codereview.appspot.com/4259065/diff/3006/src/cmd/gocheck/doc.go#newcode6
src/cmd/gocheck/doc.go:6: gocheck does syntactic and semantic analysis of Go
files and packages
s/^g/G/
or
The gocheck command ...

http://codereview.appspot.com/4259065/diff/3006/src/cmd/gocheck/doc.go#newcode36
src/cmd/gocheck/doc.go:36: disables concurrent processing.
you can drop the semicolon and what follows. the default=true is clear enough

[gri, 2011-03-10 19:20:04]

I want a driver app to hook up the typechecker. I don't want it to be just a
library. Most of the code will be outside gocheck. Also, it now takes some of
the debugging functionality (parse traces, AST dumps, etc.) that used to be in
gofmt and really shouldn't be there.

http://codereview.appspot.com/4259065/diff/3006/src/cmd/gocheck/doc.go
File src/cmd/gocheck/doc.go (right):

http://codereview.appspot.com/4259065/diff/3006/src/cmd/gocheck/doc.go#newcode6
src/cmd/gocheck/doc.go:6: gocheck does syntactic and semantic analysis of Go
files and packages
On 2011/03/10 18:42:43, r wrote:
> s/^g/G/
> or
> The gocheck command ...

Done.

http://codereview.appspot.com/4259065/diff/3006/src/cmd/gocheck/doc.go#newcode36
src/cmd/gocheck/doc.go:36: disables concurrent processing.
On 2011/03/10 18:42:43, r wrote:
> you can drop the semicolon and what follows. the default=true is clear enough

Done.

[rsc, 2011-03-10 19:21:27]

CL description on the web still says gofront

[gri, 2011-03-10 19:27:23]

that's just the web. the CL has been updated
- gri

On Thu, Mar 10, 2011 at 11:21 AM, Russ Cox <rsc@golang.org> wrote:
> CL description on the web still says gofront
>

[eds, 2011-03-10 19:36:09]

This came up before when Govet was introduced, but the name Gocheck is
in use: http://niemeyer.net/gocheck

- Evan

[gri, 2011-03-10 19:40:29]

sigh. ok, will come up with another name. thanks.
- gri

On Thu, Mar 10, 2011 at 11:36 AM, Evan Shaw <chickencha@gmail.com> wrote:
> This came up before when Govet was introduced, but the name Gocheck is
> in use: http://niemeyer.net/gocheck
>
> - Evan
>

[gri, 2011-03-10 21:00:51]

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

Please take another look.

[r, 2011-03-10 21:28:31]

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/doc.go
File src/cmd/gotype/doc.go (right):

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/doc.go#newcode14
src/cmd/gotype/doc.go:14: Given a list of file names, each file must be a
package file belonging to
s/package/source/

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/doc.go#newcode15
src/cmd/gotype/doc.go:15: the same package except for files explicitly excluded
with the -p flag.
this wording is weird. see below.

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/doc.go#newcode27
src/cmd/gotype/doc.go:27: ignore files with different package name.
the flag syntax doesn't feel right for an ignore usage message.
how about
-p pkgName
   process only those files in package pkgName

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/gotype.go
File src/cmd/gotype/gotype.go (right):

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/gotype.go#new...
src/cmd/gotype/gotype.go:23: pkgName   = flag.String("p", "", "ignore files with
different package name")
see discussion in doc.go

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/gotype.go#new...
src/cmd/gotype/gotype.go:25: verbose   = flag.Bool("v", false, "print progress
information")
"verbose" is good enough
v and progress don't align well either
or if you must
print verbose progress information

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/gotype.go#new...
src/cmd/gotype/gotype.go:29: concurrent = flag.Bool("concurrent", true, "parse
files concurrently")
leave the flag out until it is supported

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/gotype.go#new...
src/cmd/gotype/gotype.go:129: report(os.NewError(fmt.Sprintf("package %s;
expected %s", file.Name.Name, pkgName)))
include the file name too or it'll be hard to know where to look. and use %q.
file %q is in package %q not %q

[gri, 2011-03-10 23:38:23]

PTAL

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/doc.go
File src/cmd/gotype/doc.go (right):

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/doc.go#newcode14
src/cmd/gotype/doc.go:14: Given a list of file names, each file must be a
package file belonging to
On 2011/03/10 21:28:31, r wrote:
> s/package/source/

Done.

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/doc.go#newcode14
src/cmd/gotype/doc.go:14: Given a list of file names, each file must be a
package file belonging to
On 2011/03/10 21:28:31, r wrote:
> s/package/source/

Done.

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/doc.go#newcode27
src/cmd/gotype/doc.go:27: ignore files with different package name.
On 2011/03/10 21:28:31, r wrote:
> the flag syntax doesn't feel right for an ignore usage message.
> how about
> -p pkgName
>    process only those files in package pkgName

Done.

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/gotype.go
File src/cmd/gotype/gotype.go (right):

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/gotype.go#new...
src/cmd/gotype/gotype.go:23: pkgName   = flag.String("p", "", "ignore files with
different package name")
On 2011/03/10 21:28:31, r wrote:
> see discussion in doc.go

Done.

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/gotype.go#new...
src/cmd/gotype/gotype.go:25: verbose   = flag.Bool("v", false, "print progress
information")
On 2011/03/10 21:28:31, r wrote:
> "verbose" is good enough
> v and progress don't align well either
> or if you must
> print verbose progress information

Done.

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/gotype.go#new...
src/cmd/gotype/gotype.go:29: concurrent = flag.Bool("concurrent", true, "parse
files concurrently")
On 2011/03/10 21:28:31, r wrote:
> leave the flag out until it is supported

Done.

http://codereview.appspot.com/4259065/diff/25001/src/cmd/gotype/gotype.go#new...
src/cmd/gotype/gotype.go:129: report(os.NewError(fmt.Sprintf("package %s;
expected %s", file.Name.Name, pkgName)))
On 2011/03/10 21:28:31, r wrote:
> include the file name too or it'll be hard to know where to look. and use %q.
> file %q is in package %q not %q

The error message was borrowed from 6g.

Done. This is likely going to change again when this check is moving elsewhere
(and probably will assume the form of other parser errors).

[r, 2011-03-11 00:02:34]

LGTM

http://codereview.appspot.com/4259065/diff/29001/src/cmd/gotype/doc.go
File src/cmd/gotype/doc.go (right):

http://codereview.appspot.com/4259065/diff/29001/src/cmd/gotype/doc.go#newcode36
src/cmd/gotype/doc.go:36: process files concurrently.
this is now gone

[rsc, 2011-03-11 15:09:55]

On Thu, Mar 10, 2011 at 14:27, Robert Griesemer <gri@golang.org> wrote:
> that's just the web. the CL has been updated

i assume that you moved the CL using hg clpatch
from one machine to another.  be aware that if you
move it again clpatch fetches the description from
the web, so you will get the stale one.

russ

[rsc, 2011-03-11 15:16:57]

LGTM

http://codereview.appspot.com/4259065/diff/29001/src/cmd/Makefile
File src/cmd/Makefile (right):

http://codereview.appspot.com/4259065/diff/29001/src/cmd/Makefile#newcode45
src/cmd/Makefile:45: gotype\
sort down

http://codereview.appspot.com/4259065/diff/29001/src/cmd/gotype/doc.go
File src/cmd/gotype/doc.go (right):

http://codereview.appspot.com/4259065/diff/29001/src/cmd/gotype/doc.go#newcode45
src/cmd/gotype/doc.go:45: To check the files file.go, old.saved, and .invisible:
s/invisible/ignored/

the files are not invisible, just omitted by
bad tools like ls.

http://codereview.appspot.com/4259065/diff/29001/src/cmd/gotype/gotype.go
File src/cmd/gotype/gotype.go (right):

http://codereview.appspot.com/4259065/diff/29001/src/cmd/gotype/gotype.go#new...
src/cmd/gotype/gotype.go:185: processPackage([]string{os.Stdin.Name()})
I don't believe you can open os.Stdin.Name() on windows,
and you can't even open it on Unix in some conditions
(for example, if stdin is an open network connection).

Would be more reliable to pass a *os.File, string pair in this case.

[gri, 2011-03-11 18:26:17]

On Fri, Mar 11, 2011 at 7:16 AM,  <rsc@golang.org> wrote:
>
http://codereview.appspot.com/4259065/diff/29001/src/cmd/gotype/gotype.go#new...
> src/cmd/gotype/gotype.go:185: processPackage([]string{os.Stdin.Name()})
> I don't believe you can open os.Stdin.Name() on windows,
> and you can't even open it on Unix in some conditions
> (for example, if stdin is an open network connection).
>
> Would be more reliable to pass a *os.File, string pair in this case.

Let me fix this in a separate CL. I think I need to restructure a bit.
Everything else fixed. Thanks for the review.

- Robert

[gri, 2011-03-11 18:27:29]

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

gotype: commandline tool to typecheck go programs

First version. Handles scope analysis only at the
moment.

R=rsc, r, eds
CC=golang-dev
http://codereview.appspot.com/4259065

Committer: Robert Griesemer <gri@golang.org>