code review 7492045: go/types: more internal cleanups

go/types: more internal cleanups

[gri, 2013-03-07 18:16:35]

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

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2013-03-07 18:22:23]

https://codereview.appspot.com/7492045/diff/5001/src/pkg/go/types/stdlib_test.go
File src/pkg/go/types/stdlib_test.go (right):

https://codereview.appspot.com/7492045/diff/5001/src/pkg/go/types/stdlib_test...
src/pkg/go/types/stdlib_test.go:46: "code.google.com": true,
these shouldn't be in $GOROOT/src anyway these days.  If a builder has these, it
should fail so we could clean it.

If you did want to keep this, I'd ask why not github.com and mydomain.com too. 
Or anything with a dot in it.

[gri, 2013-03-07 18:45:32]

PTAL.
- gri

https://codereview.appspot.com/7492045/diff/5001/src/pkg/go/types/stdlib_test.go
File src/pkg/go/types/stdlib_test.go (right):

https://codereview.appspot.com/7492045/diff/5001/src/pkg/go/types/stdlib_test...
src/pkg/go/types/stdlib_test.go:46: "code.google.com": true,
On 2013/03/07 18:22:23, bradfitz wrote:
> these shouldn't be in $GOROOT/src anyway these days.  If a builder has these,
it
> should fail so we could clean it.
> 
> If you did want to keep this, I'd ask why not http://github.com and
http://mydomain.com too. 
> Or anything with a dot in it.

Fair enough. Just tried to be resistent against my own misconfiguration... :-)

Reverted.

[adonovan, 2013-03-07 18:53:58]

https://codereview.appspot.com/7492045/diff/9001/src/pkg/go/types/stmt.go
File src/pkg/go/types/stmt.go (right):

https://codereview.appspot.com/7492045/diff/9001/src/pkg/go/types/stmt.go#new...
src/pkg/go/types/stmt.go:92: switch obj := obj.(type) {
What happened to the defer-based scheme in your other CL?
Did you abandon it or is it still pending?

https://codereview.appspot.com/7492045/diff/9001/src/pkg/go/types/stmt.go#new...
src/pkg/go/types/stmt.go:219: check.errorf(e.Pos(), "cannot declare %s", e)
e is an ast.Expr; do these print nicely?  I don't think so.

[gri, 2013-03-07 19:04:28]

https://codereview.appspot.com/7492045/diff/9001/src/pkg/go/types/stmt.go
File src/pkg/go/types/stmt.go (right):

https://codereview.appspot.com/7492045/diff/9001/src/pkg/go/types/stmt.go#new...
src/pkg/go/types/stmt.go:92: switch obj := obj.(type) {
On 2013/03/07 18:53:58, adonovan wrote:
> What happened to the defer-based scheme in your other CL?
> Did you abandon it or is it still pending?

See code just above. The defer wasn't necessary after all and the code got much
simpler.

https://codereview.appspot.com/7492045/diff/9001/src/pkg/go/types/stmt.go#new...
src/pkg/go/types/stmt.go:219: check.errorf(e.Pos(), "cannot declare %s", e)
On 2013/03/07 18:53:58, adonovan wrote:
> e is an ast.Expr; do these print nicely?  I don't think so.

yep they do. used all over the place.

[adonovan, 2013-03-07 19:16:12]

On 2013/03/07 19:04:28, gri wrote:
> https://codereview.appspot.com/7492045/diff/9001/src/pkg/go/types/stmt.go
> File src/pkg/go/types/stmt.go (right):
> 
>
https://codereview.appspot.com/7492045/diff/9001/src/pkg/go/types/stmt.go#new...
> src/pkg/go/types/stmt.go:92: switch obj := obj.(type) {
> On 2013/03/07 18:53:58, adonovan wrote:
> > What happened to the defer-based scheme in your other CL?
> > Did you abandon it or is it still pending?
> 
> See code just above. The defer wasn't necessary after all and the code got
much
> simpler.
> 
>
https://codereview.appspot.com/7492045/diff/9001/src/pkg/go/types/stmt.go#new...
> src/pkg/go/types/stmt.go:219: check.errorf(e.Pos(), "cannot declare %s", e)
> On 2013/03/07 18:53:58, adonovan wrote:
> > e is an ast.Expr; do these print nicely?  I don't think so.
> 
> yep they do. used all over the place.

LGTM

[gri, 2013-03-07 19:17:33]

*** Submitted as https://code.google.com/p/go/source/detail?r=3a15792a6542 ***

go/types: more internal cleanups

R=adonovan, bradfitz
CC=golang-dev
https://codereview.appspot.com/7492045