|
|
Created:
13 years, 7 months ago by gri Modified:
13 years, 7 months ago Reviewers:
CC:
golang-dev, r, bradfitz Visibility:
Public. |
Descriptiongo/ast, parser: remember short variable decls. w/ correspoding ident objects
The ast.Object's Decl field pointed back to the corresponding declaration for
all but short variable declarations. Now remember corresponding assignment
statement in the Decl field.
Also: simplified some code for parsing select statements.
Patch Set 1 #Patch Set 2 : diff -r 561eb3ec7eb5 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 561eb3ec7eb5 https://go.googlecode.com/hg/ #Patch Set 4 : diff -r 561eb3ec7eb5 https://go.googlecode.com/hg/ #Patch Set 5 : diff -r 30cb39ed6081 https://go.googlecode.com/hg/ #Patch Set 6 : diff -r 67b329eb1ecb https://go.googlecode.com/hg/ #MessagesTotal messages: 5
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
I don't claim to understand this code, but sure looks tricky for no new tests. :-) On Mon, Dec 19, 2011 at 2:02 PM, <gri@golang.org> wrote: > Reviewers: golang-dev_googlegroups.com, > > Message: > Hello golang-dev@googlegroups.com, > > I'd like you to review this change to > https://go.googlecode.com/hg/ > > > Description: > go/ast, parser: remember short variable decls. w/ correspoding ident > objects > > The ast.Object's Decl field pointed back to the corresponding > declaration for > all but short variable declarations. Now remember corresponding > assignment > statement in the Decl field. > > Also: simplified some code for parsing select statements. > > Please review this at http://codereview.appspot.com/**5492072/<http://codereview.appspot.com/5492072/> > > Affected files: > M src/pkg/go/ast/scope.go > M src/pkg/go/parser/parser.go > > > Index: src/pkg/go/ast/scope.go > ==============================**==============================**======= > --- a/src/pkg/go/ast/scope.go > +++ b/src/pkg/go/ast/scope.go > @@ -80,7 +80,7 @@ > type Object struct { > Kind ObjKind > Name string // declared name > - Decl interface{} // corresponding Field, XxxSpec, FuncDecl, or > LabeledStmt; or nil > + Decl interface{} // corresponding Field, XxxSpec, FuncDecl, > LabeledStmt, or AssignStmt; or nil > Data interface{} // object-specific data; or nil > Type interface{} // place holder for type information; may be nil > } > @@ -125,6 +125,12 @@ > if d.Label.Name == name { > return d.Label.Pos() > } > + case *AssignStmt: > + for _, x := range d.Lhs { > + if ident, isIdent := x.(*Ident); isIdent && > ident.Name == name { > + return ident.Pos() > + } > + } > } > return token.NoPos > } > Index: src/pkg/go/parser/parser.go > ==============================**==============================**======= > --- a/src/pkg/go/parser/parser.go > +++ b/src/pkg/go/parser/parser.go > @@ -144,28 +144,31 @@ > } > } > > -func (p *parser) shortVarDecl(idents []*ast.Ident) { > +func (p *parser) shortVarDecl(decl interface{}, list []ast.Expr) { > // Go spec: A short variable declaration may redeclare variables > // provided they were originally declared in the same block with > // the same type, and at least one of the non-blank variables is > new. > n := 0 // number of new variables > - for _, ident := range idents { > - assert(ident.Obj == nil, "identifier already declared or > resolved") > - obj := ast.NewObj(ast.Var, ident.Name) > - // short var declarations cannot have redeclaration errors > - // and are not global => no need to remember the respective > - // declaration > - ident.Obj = obj > - if ident.Name != "_" { > - if alt := p.topScope.Insert(obj); alt != nil { > - ident.Obj = alt // redeclaration > - } else { > - n++ // new declaration > + for _, x := range list { > + if ident, isIdent := x.(*ast.Ident); isIdent { > + assert(ident.Obj == nil, "identifier already > declared or resolved") > + obj := ast.NewObj(ast.Var, ident.Name) > + // remember corresponding assignment for other > tools > + obj.Decl = decl > + ident.Obj = obj > + if ident.Name != "_" { > + if alt := p.topScope.Insert(obj); alt != > nil { > + ident.Obj = alt // redeclaration > + } else { > + n++ // new declaration > + } > } > + } else { > + p.errorExpected(x.Pos(), "identifier") > } > } > if n == 0 && p.mode&DeclarationErrors != 0 { > - p.error(idents[0].Pos(), "no new variables on left side of > :=") > + p.error(list[0].Pos(), "no new variables on left side of > :=") > } > } > > @@ -522,7 +525,7 @@ > for i, x := range list { > ident, isIdent := x.(*ast.Ident) > if !isIdent { > - pos := x.(ast.Expr).Pos() > + pos := x.Pos() > p.errorExpected(pos, "identifier") > ident = &ast.Ident{pos, "_", nil} > } > @@ -1400,10 +1403,11 @@ > } else { > y = p.parseRhsList() > } > + as := &ast.AssignStmt{x, pos, tok, y} > if tok == token.DEFINE { > - p.shortVarDecl(p.**makeIdentList(x)) > + p.shortVarDecl(as, x) > } > - return &ast.AssignStmt{x, pos, tok, y}, isRange > + return as, isRange > } > > if len(x) > 1 { > @@ -1715,34 +1719,28 @@ > comm = &ast.SendStmt{lhs[0], arrow, rhs} > } else { > // RecvStmt > - pos := p.pos > - tok := p.tok > - var rhs ast.Expr > - if tok == token.ASSIGN || tok == token.DEFINE { > + if tok := p.tok; tok == token.ASSIGN || tok == > token.DEFINE { > // RecvStmt with assignment > if len(lhs) > 2 { > p.errorExpected(lhs[0].Pos(), "1 or > 2 expressions") > // continue with first two > expressions > lhs = lhs[0:2] > } > + pos := p.pos > p.next() > - rhs = p.parseRhs() > - if tok == token.DEFINE && lhs != nil { > - p.shortVarDecl(p.** > makeIdentList(lhs)) > + rhs := p.parseRhs() > + as := &ast.AssignStmt{lhs, pos, tok, > []ast.Expr{rhs}} > + if tok == token.DEFINE { > + p.shortVarDecl(as, lhs) > } > + comm = as > } else { > - // rhs must be single receive operation > + // lhs must be single receive operation > if len(lhs) > 1 { > p.errorExpected(lhs[0].Pos(), "1 > expression") > // continue with first expression > } > - rhs = lhs[0] > - lhs = nil // there is no lhs > - } > - if lhs != nil { > - comm = &ast.AssignStmt{lhs, pos, tok, > []ast.Expr{rhs}} > - } else { > - comm = &ast.ExprStmt{rhs} > + comm = &ast.ExprStmt{lhs[0]} > } > } > } else { > > >
Sign in to reply to this message.
Yes, agreed. The effect of it can be seen when running gotype -ast <some program>, and there are some tests in the gotype test suite, but there should be more. On the list. - Robert On Tue, Dec 20, 2011 at 8:49 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > I don't claim to understand this code, but sure looks tricky for no new > tests. :-) > > > On Mon, Dec 19, 2011 at 2:02 PM, <gri@golang.org> wrote: >> >> Reviewers: golang-dev_googlegroups.com, >> >> Message: >> Hello golang-dev@googlegroups.com, >> >> I'd like you to review this change to >> https://go.googlecode.com/hg/ >> >> >> Description: >> go/ast, parser: remember short variable decls. w/ correspoding ident >> objects >> >> The ast.Object's Decl field pointed back to the corresponding >> declaration for >> all but short variable declarations. Now remember corresponding >> assignment >> statement in the Decl field. >> >> Also: simplified some code for parsing select statements. >> >> Please review this at http://codereview.appspot.com/5492072/ >> >> Affected files: >> M src/pkg/go/ast/scope.go >> M src/pkg/go/parser/parser.go >> >> >> Index: src/pkg/go/ast/scope.go >> =================================================================== >> --- a/src/pkg/go/ast/scope.go >> +++ b/src/pkg/go/ast/scope.go >> @@ -80,7 +80,7 @@ >> type Object struct { >> Kind ObjKind >> Name string // declared name >> - Decl interface{} // corresponding Field, XxxSpec, FuncDecl, or >> LabeledStmt; or nil >> + Decl interface{} // corresponding Field, XxxSpec, FuncDecl, >> LabeledStmt, or AssignStmt; or nil >> Data interface{} // object-specific data; or nil >> Type interface{} // place holder for type information; may be nil >> } >> @@ -125,6 +125,12 @@ >> if d.Label.Name == name { >> return d.Label.Pos() >> } >> + case *AssignStmt: >> + for _, x := range d.Lhs { >> + if ident, isIdent := x.(*Ident); isIdent && >> ident.Name == name { >> + return ident.Pos() >> + } >> + } >> } >> return token.NoPos >> } >> Index: src/pkg/go/parser/parser.go >> =================================================================== >> --- a/src/pkg/go/parser/parser.go >> +++ b/src/pkg/go/parser/parser.go >> @@ -144,28 +144,31 @@ >> } >> } >> >> -func (p *parser) shortVarDecl(idents []*ast.Ident) { >> +func (p *parser) shortVarDecl(decl interface{}, list []ast.Expr) { >> // Go spec: A short variable declaration may redeclare variables >> // provided they were originally declared in the same block with >> // the same type, and at least one of the non-blank variables is >> new. >> n := 0 // number of new variables >> - for _, ident := range idents { >> - assert(ident.Obj == nil, "identifier already declared or >> resolved") >> - obj := ast.NewObj(ast.Var, ident.Name) >> - // short var declarations cannot have redeclaration errors >> - // and are not global => no need to remember the >> respective >> - // declaration >> - ident.Obj = obj >> - if ident.Name != "_" { >> - if alt := p.topScope.Insert(obj); alt != nil { >> - ident.Obj = alt // redeclaration >> - } else { >> - n++ // new declaration >> + for _, x := range list { >> + if ident, isIdent := x.(*ast.Ident); isIdent { >> + assert(ident.Obj == nil, "identifier already >> declared or resolved") >> + obj := ast.NewObj(ast.Var, ident.Name) >> + // remember corresponding assignment for other >> tools >> + obj.Decl = decl >> + ident.Obj = obj >> + if ident.Name != "_" { >> + if alt := p.topScope.Insert(obj); alt != >> nil { >> + ident.Obj = alt // redeclaration >> + } else { >> + n++ // new declaration >> + } >> } >> + } else { >> + p.errorExpected(x.Pos(), "identifier") >> } >> } >> if n == 0 && p.mode&DeclarationErrors != 0 { >> - p.error(idents[0].Pos(), "no new variables on left side of >> :=") >> + p.error(list[0].Pos(), "no new variables on left side of >> :=") >> } >> } >> >> @@ -522,7 +525,7 @@ >> for i, x := range list { >> ident, isIdent := x.(*ast.Ident) >> if !isIdent { >> - pos := x.(ast.Expr).Pos() >> + pos := x.Pos() >> p.errorExpected(pos, "identifier") >> ident = &ast.Ident{pos, "_", nil} >> } >> @@ -1400,10 +1403,11 @@ >> } else { >> y = p.parseRhsList() >> } >> + as := &ast.AssignStmt{x, pos, tok, y} >> if tok == token.DEFINE { >> - p.shortVarDecl(p.makeIdentList(x)) >> + p.shortVarDecl(as, x) >> } >> - return &ast.AssignStmt{x, pos, tok, y}, isRange >> + return as, isRange >> } >> >> if len(x) > 1 { >> @@ -1715,34 +1719,28 @@ >> comm = &ast.SendStmt{lhs[0], arrow, rhs} >> } else { >> // RecvStmt >> - pos := p.pos >> - tok := p.tok >> - var rhs ast.Expr >> - if tok == token.ASSIGN || tok == token.DEFINE { >> + if tok := p.tok; tok == token.ASSIGN || tok == >> token.DEFINE { >> // RecvStmt with assignment >> if len(lhs) > 2 { >> p.errorExpected(lhs[0].Pos(), "1 or >> 2 expressions") >> // continue with first two >> expressions >> lhs = lhs[0:2] >> } >> + pos := p.pos >> p.next() >> - rhs = p.parseRhs() >> - if tok == token.DEFINE && lhs != nil { >> - >> p.shortVarDecl(p.makeIdentList(lhs)) >> + rhs := p.parseRhs() >> + as := &ast.AssignStmt{lhs, pos, tok, >> []ast.Expr{rhs}} >> + if tok == token.DEFINE { >> + p.shortVarDecl(as, lhs) >> } >> + comm = as >> } else { >> - // rhs must be single receive operation >> + // lhs must be single receive operation >> if len(lhs) > 1 { >> p.errorExpected(lhs[0].Pos(), "1 >> expression") >> // continue with first expression >> } >> - rhs = lhs[0] >> - lhs = nil // there is no lhs >> - } >> - if lhs != nil { >> - comm = &ast.AssignStmt{lhs, pos, tok, >> []ast.Expr{rhs}} >> - } else { >> - comm = &ast.ExprStmt{rhs} >> + comm = &ast.ExprStmt{lhs[0]} >> } >> } >> } else { >> >> >
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=94fea4e3fa40 *** go/ast, parser: remember short variable decls. w/ correspoding ident objects The ast.Object's Decl field pointed back to the corresponding declaration for all but short variable declarations. Now remember corresponding assignment statement in the Decl field. Also: simplified some code for parsing select statements. R=golang-dev, r, bradfitz CC=golang-dev http://codereview.appspot.com/5492072
Sign in to reply to this message.
|