code review 1994041: go AST: First step towards augmenting AST with full typ...

go AST: First step towards augmenting AST with full type information.

- change ast.Ident back to contain the name and adjust all dependent code
- identifier object information will be added again through an optional
  typechecker phase (in the works).
- remove tracking of scopes in parser - it's easier to do this in a separate
  phase (in the works)
- simplify generation of popup information in godoc

As a result of this change, the currently shown popup information
(const, var, type, func, followed by identifier name) will not be
shown anymore temporarily.

[gri, 2010-08-13 00:46:50]

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

I'd like you to review this change.

[rsc1, 2010-08-13 02:52:24]

LGTM

http://codereview.appspot.com/1994041/diff/13001/14016
File src/pkg/go/ast/ast.go (right):

http://codereview.appspot.com/1994041/diff/13001/14016#newcode633
src/pkg/go/ast/ast.go:633: Type    Expr          // *Ident, *ParenExpr,
*SelectorExpr, *StarExpr, or any of the *xxxTypes
s/xxx/Xxx/

http://codereview.appspot.com/1994041/diff/13001/14018
File src/pkg/go/ast/scope.go (right):

http://codereview.appspot.com/1994041/diff/13001/14018#newcode61
src/pkg/go/ast/scope.go:61: // vatriables. Thus for now, we store scope entries
variables

http://codereview.appspot.com/1994041/diff/13001/14018#newcode70
src/pkg/go/ast/scope.go:70: const n = 4 // a reasonable maximum number of
objects per scope
why is 4 reasonable as a maximum?
or should this say minimum?

http://codereview.appspot.com/1994041/diff/13001/14018#newcode77
src/pkg/go/ast/scope.go:77: if n < cap(s.Objects) {
Fine as is, but elsewhere in the tree this is usually written without an else,
as below, so that the growing
of the len is not duplicated.  I had to read it twice
to figure out where it was.

if n >= cap {
    new := make([]*Object, n, 2*n)
    copy
}
s.Objects = s.Objects[0 : n+1]
s.Objects[n] = obj

[gri, 2010-08-13 17:37:25]

On Thu, Aug 12, 2010 at 7:52 PM, <rsc@google.com> wrote:

> LGTM
>
> http://codereview.appspot.com/1994041/diff/13001/14016
> File src/pkg/go/ast/ast.go (right):
>
> http://codereview.appspot.com/1994041/diff/13001/14016#newcode633
> src/pkg/go/ast/ast.go:633: Type    Expr          // *Ident, *ParenExpr,
> *SelectorExpr, *StarExpr, or any of the *xxxTypes
> s/xxx/Xxx/
>
done

>
> http://codereview.appspot.com/1994041/diff/13001/14018
> File src/pkg/go/ast/scope.go (right):
>
> http://codereview.appspot.com/1994041/diff/13001/14018#newcode61
> src/pkg/go/ast/scope.go:61: // vatriables. Thus for now, we store scope
> entries
> variables
>
done

>
> http://codereview.appspot.com/1994041/diff/13001/14018#newcode70
> src/pkg/go/ast/scope.go:70: const n = 4 // a reasonable maximum number
> of objects per scope
> why is 4 reasonable as a maximum?
> or should this say minimum?
>
changed

>
> http://codereview.appspot.com/1994041/diff/13001/14018#newcode77
> src/pkg/go/ast/scope.go:77: if n < cap(s.Objects) {
> Fine as is, but elsewhere in the tree this is usually written without an
> else, as below, so that the growing
> of the len is not duplicated.  I had to read it twice
> to figure out where it was.
>
> if n >= cap {
>    new := make([]*Object, n, 2*n)
>    copy
> }
> s.Objects = s.Objects[0 : n+1]
> s.Objects[n] = obj


changed to use canonical pattern

>


> http://codereview.appspot.com/1994041/show
>

thx for the quick review
- gri

[gri, 2010-08-13 17:42:24]

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

go AST: First step towards augmenting AST with full type information.

- change ast.Ident back to contain the name and adjust all dependent code
- identifier object information will be added again through an optional
  typechecker phase (in the works).
- remove tracking of scopes in parser - it's easier to do this in a separate
  phase (in the works)
- in godoc, generate popup info table directly instead of through a formatter
  for simpler data flow (at the expense of a little bit more code)

Runs all tests.

As a result of this change, the currently shown popup information
(const, var, type, func, followed by identifier name) will not be
shown anymore temporarily.

R=rsc
CC=golang-dev
http://codereview.appspot.com/1994041

Committer: Robert Griesemer <gri@golang.org>