FAQ
PTAL


http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/check.go
File src/pkg/exp/types/staging/check.go (right):

http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/check.go#newcode1
src/pkg/exp/types/staging/check.go:1: // Copyright P011 The Go Authors.
All rights reserved.
On 2012/09/17 20:28:46, rsc wrote:
s/P/2/
Done.

http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/check.go#newcode24
src/pkg/exp/types/staging/check.go:24: func (check *checker)
declare(scope *ast.Scope, kind ast.ObjKind, ident *ast.Ident, decl
ast.Decl) {
On 2012/09/17 20:28:46, rsc wrote:
Could you add short doc comments to each of these top-level functions and
methods? I can kind of guess what they're doing, but it would help to have
confirmation of my guesses.
Done.

http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/check.go#newcode89
src/pkg/exp/types/staging/check.go:89: // TODO(gri) FIX THIS! Quadratic
algorithm - just to get going for now.
On 2012/09/17 20:28:46, rsc wrote:
I was confused by this because the code here is not quadratic. It's the fact
that it gets called in a loop that causes the problem. If you add a
map[*ast.ValueSpec][]ast.Expr to the checker, you could tweak this code to
record all the results on the first call and then use the map on
subsequent
calls.
Modified comment.

http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/check.go#newcode255
src/pkg/exp/types/staging/check.go:255: if obj, local =
lookup(pkg.Scope, ident.Name); obj != nil {
On 2012/09/17 20:28:46, rsc wrote:
I'm a little confused. Is ident.Obj not set for some reason?
It is. I don't know what I was thinking. Adjusted.

http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/const.go
File src/pkg/exp/types/staging/const.go (right):

http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/const.go#newcode1
src/pkg/exp/types/staging/const.go:1: // Copyright 2012 The Go Authors.
All rights reserved.
On 2012/09/17 20:28:46, rsc wrote:
No need to update copyright years.
Understood. This was essentially a complete rewrite. Changed back.

http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/const.go#newcode66
src/pkg/exp/types/staging/const.go:66: func normalizeIntConst(x
*big.Int) interface{} {
On 2012/09/17 20:28:46, rsc wrote:
Small doc comments on all top-level funcs and methods please.
Done.

http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/const.go#newcode156
src/pkg/exp/types/staging/const.go:156: return 0 <= x && x < 1<<intBits
On 2012/09/17 20:28:46, rsc wrote:
Use x <= 1<<intBits-1 to avoid problems when intBits = 64.
Done.

http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/const.go#newcode166
src/pkg/exp/types/staging/const.go:166: return 0 <= x /* && x <
1<<ptrBits */
On 2012/09/17 20:28:46, rsc wrote:
x <= 1<<ptrBits-1
Done.

http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/const.go#newcode176
src/pkg/exp/types/staging/const.go:176: return true
On 2012/09/17 20:28:46, rsc wrote:
In the doc comment please explain the possible input kinds. Seeing
UntypedInt
makes me wonder why UntypedFloat etc don't appear.
Expanded code (easier than special case it).

http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/const.go#newcode181
src/pkg/exp/types/staging/const.go:181: case Uint64:
On 2012/09/17 20:28:46, rsc wrote:
need case Uint here too when intBits==64.
Done.

http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/const.go#newcode246
src/pkg/exp/types/staging/const.go:246: switch x := x.(type) {
On 2012/09/17 20:28:46, rsc wrote:
I wonder if some tables would simplify some of these functions. If you had a
function to measure complexity (1=bool, 2=int64, 3=big.Int, 4=big.Rat,
5=complex), you could at least do
if complexity(x) > complexity(y) {
y, x = matchConst(y, x)
return x, y
}
and then only need half as many cases here.
Done. Saves about a dozen lines.

http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/const.go#newcode324
src/pkg/exp/types/staging/const.go:324: // Returns nil if a
division-by-zero error occurs.
On 2012/09/17 20:28:46, rsc wrote:
Does more than that.
Done.

http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/errors.go
File src/pkg/exp/types/staging/errors.go (right):

http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/errors.go#newcode20
src/pkg/exp/types/staging/errors.go:20: func assert(p bool) {
On 2012/09/17 20:28:46, rsc wrote:
func/method comments
Hopefully assert is just for debugging and the eventual API will
return errors
always, even 'internal error' errors.
yes - but for now it's useful

http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/stubs.go
File src/pkg/exp/types/staging/stubs.go (right):

http://codereview.appspot.com/6500114/diff/10010/src/pkg/exp/types/staging/stubs.go#newcode29
src/pkg/exp/types/staging/stubs.go:29: func (check *checker)
assignNtoM(lhs, rhs []ast.Expr, decl bool, iota int) {
On 2012/09/17 20:28:46, rsc wrote:
comments
Done.

http://codereview.appspot.com/6500114/

Search Discussions

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedSep 18, '12 at 11:16p
activeSep 18, '12 at 11:16p
posts1
users1
websitegolang.org

1 user in discussion

Gri: 1 post

People

Translate

site design / logo © 2021 Grokbase