code review 5266045: gofix -r mapdelete

gofix -r mapdelete

[rsc, 2011-10-17 20:07:45]

Hello golang-dev@googlegroups.com,

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

[r, 2011-10-17 20:12:29]

LGTM except for the ringers

http://codereview.appspot.com/5266045/diff/4001/src/cmd/goinstall/main.go
File src/cmd/goinstall/main.go (right):

http://codereview.appspot.com/5266045/diff/4001/src/cmd/goinstall/main.go#new...
src/cmd/goinstall/main.go:202: if err == nil && tree.Goroot &&
isStandardPath(pkg) && parent != "" {
this seems unrelated.

http://codereview.appspot.com/5266045/diff/4001/src/cmd/gotest/gotest.go
File src/cmd/gotest/gotest.go (right):

http://codereview.appspot.com/5266045/diff/4001/src/cmd/gotest/gotest.go#newc...
src/cmd/gotest/gotest.go:162: GL = []string{gl, "-L", "_test", "-L",
"/Users/rsc/g/pkg/darwin_amd64", "_testmain." + O}
this is unrelated.

[adg, 2011-10-17 20:47:53]

Oh wow, I really like how this has turned out.

On 18 October 2011 07:07,  <rsc@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:
> gofix -r mapdelete
>
> Please review this at http://codereview.appspot.com/5266045/
>
> Affected files:
>  M src/cmd/cgo/gcc.go
>  M src/cmd/gofmt/rewrite.go
>  M src/cmd/goinstall/main.go
>  M src/cmd/gotest/gotest.go
>  M src/pkg/debug/dwarf/type.go
>  M src/pkg/exp/types/check_test.go
>  M src/pkg/exp/types/universe.go
>  M src/pkg/gob/decode.go
>  M src/pkg/gob/type.go
>  M src/pkg/http/fcgi/child.go
>  M src/pkg/http/persist.go
>  M src/pkg/http/transfer.go
>  M src/pkg/http/transport.go
>  M src/pkg/net/fd.go
>  M src/pkg/net/fd_linux.go
>  M src/pkg/net/textproto/header.go
>  M src/pkg/net/textproto/pipeline.go
>  M src/pkg/old/netchan/export.go
>  M src/pkg/old/netchan/import.go
>  M src/pkg/os/inotify/inotify_linux.go
>  M src/pkg/reflect/type.go
>  M src/pkg/rpc/client.go
>  M src/pkg/rpc/jsonrpc/client.go
>  M src/pkg/rpc/jsonrpc/server.go
>  M src/pkg/syscall/syscall_unix.go
>  M src/pkg/unicode/maketables.go
>  M src/pkg/unicode/script_test.go
>  M src/pkg/url/url.go
>  M src/pkg/xml/xml.go
>  M test/nil.go
>
>
> Index: src/cmd/cgo/gcc.go
> ===================================================================
> --- a/src/cmd/cgo/gcc.go
> +++ b/src/cmd/cgo/gcc.go
> @@ -588,7 +588,7 @@
>                                n.Const =
> strconv.Itoa64(n.Type.EnumValues[k])
>                                // Remove injected enum to ensure the value
> will deep-compare
>                                // equally in future loads of the same
> constant.
> -                               n.Type.EnumValues[k] = 0, false
> +                               delete(n.Type.EnumValues, k)
>                        } else if n.Kind == "const" && i < len(enumVal) {
>                                n.Const = strconv.Itoa64(enumVal[i])
>                        }
> Index: src/cmd/gofmt/rewrite.go
> ===================================================================
> --- a/src/cmd/gofmt/rewrite.go
> +++ b/src/cmd/gofmt/rewrite.go
> @@ -65,7 +65,7 @@
>                        return reflect.Value{}
>                }
>                for k := range m {
> -                       m[k] = reflect.Value{}, false
> +                       delete(m, k)
>                }
>                val = apply(f, val)
>                if match(m, pat, val) {
> Index: src/cmd/goinstall/main.go
> ===================================================================
> --- a/src/cmd/goinstall/main.go
> +++ b/src/cmd/goinstall/main.go
> @@ -199,7 +199,7 @@
>        // If remote, download or update it.
>        tree, pkg, err := build.FindTree(pkg)
>        // Don't build the standard library.
> -       if err == nil && tree.Goroot && isStandardPath(pkg) {
> +       if err == nil && tree.Goroot && isStandardPath(pkg) && parent != ""
> {
>                if parent == "" {
>                        errorf("%s: can not goinstall the standard
> library\n", pkg)
>                } else {
> Index: src/cmd/gotest/gotest.go
> ===================================================================
> --- a/src/cmd/gotest/gotest.go
> +++ b/src/cmd/gotest/gotest.go
> @@ -159,7 +159,7 @@
>        if gl == "" {
>                gl = O + "l"
>        }
> -       GL = []string{gl, "-L", "_test", "_testmain." + O}
> +       GL = []string{gl, "-L", "_test", "-L",
> "/Users/rsc/g/pkg/darwin_amd64", "_testmain." + O}
>
>        // Silence make on Linux
>        addEnv("MAKEFLAGS", "")
> Index: src/pkg/debug/dwarf/type.go
> ===================================================================
> --- a/src/pkg/debug/dwarf/type.go
> +++ b/src/pkg/debug/dwarf/type.go
> @@ -579,6 +579,6 @@
>        // If the parse fails, take the type out of the cache
>        // so that the next call with this offset doesn't hit
>        // the cache and return success.
> -       d.typeCache[off] = nil, false
> +       delete(d.typeCache, off)
>        return nil, err
>  }
> Index: src/pkg/exp/types/check_test.go
> ===================================================================
> --- a/src/pkg/exp/types/check_test.go
> +++ b/src/pkg/exp/types/check_test.go
> @@ -154,7 +154,7 @@
>                                continue
>                        }
>                        // we have a match - eliminate this error
> -                       expected[pos] = "", false
> +                       delete(expected, pos)
>                } else {
>                        // To keep in mind when analyzing failed test output:
>                        // If the same error position occurs multiple times
> in errors,
> Index: src/pkg/exp/types/universe.go
> ===================================================================
> --- a/src/pkg/exp/types/universe.go
> +++ b/src/pkg/exp/types/universe.go
> @@ -81,6 +81,7 @@
>        defFun("close")
>        defFun("complex")
>        defFun("copy")
> +       defFun("delete")
>        defFun("imag")
>        defFun("len")
>        defFun("make")
> Index: src/pkg/gob/decode.go
> ===================================================================
> --- a/src/pkg/gob/decode.go
> +++ b/src/pkg/gob/decode.go
> @@ -1154,7 +1154,7 @@
>                decoderMap[remoteId] = enginePtr
>                *enginePtr, err = dec.compileDec(remoteId, ut)
>                if err != nil {
> -                       decoderMap[remoteId] = nil, false
> +                       delete(decoderMap, remoteId)
>                }
>        }
>        return
> @@ -1179,7 +1179,7 @@
>                        *enginePtr, err = dec.compileIgnoreSingle(wireId)
>                }
>                if err != nil {
> -                       dec.ignorerCache[wireId] = nil, false
> +                       delete(dec.ignorerCache, wireId)
>                }
>        }
>        return
> Index: src/pkg/gob/type.go
> ===================================================================
> --- a/src/pkg/gob/type.go
> +++ b/src/pkg/gob/type.go
> @@ -405,7 +405,7 @@
>        var type0, type1 gobType
>        defer func() {
>                if err != nil {
> -                       types[rt] = nil, false
> +                       delete(types, rt)
>                }
>        }()
>        // Install the top-level type before the subtypes (e.g. struct before
> Index: src/pkg/http/fcgi/child.go
> ===================================================================
> --- a/src/pkg/http/fcgi/child.go
> +++ b/src/pkg/http/fcgi/child.go
> @@ -194,7 +194,7 @@
>                case typeData:
>                        // If the filter role is implemented, read the data
> stream here.
>                case typeAbortRequest:
> -                       requests[rec.h.Id] = nil, false
> +                       delete(requests, rec.h.Id)
>                        c.conn.writeEndRequest(rec.h.Id, 0,
> statusRequestComplete)
>                        if !req.keepConn {
>                                // connection will close upon return
> Index: src/pkg/http/persist.go
> ===================================================================
> --- a/src/pkg/http/persist.go
> +++ b/src/pkg/http/persist.go
> @@ -165,7 +165,7 @@
>        // Retrieve the pipeline ID of this request/response pair
>        sc.lk.Lock()
>        id, ok := sc.pipereq[req]
> -       sc.pipereq[req] = 0, false
> +       delete(sc.pipereq, req)
>        if !ok {
>                sc.lk.Unlock()
>                return ErrPipeline
> @@ -353,7 +353,7 @@
>        // Retrieve the pipeline ID of this request/response pair
>        cc.lk.Lock()
>        id, ok := cc.pipereq[req]
> -       cc.pipereq[req] = 0, false
> +       delete(cc.pipereq, req)
>        if !ok {
>                cc.lk.Unlock()
>                return nil, ErrPipeline
> Index: src/pkg/http/transfer.go
> ===================================================================
> --- a/src/pkg/http/transfer.go
> +++ b/src/pkg/http/transfer.go
> @@ -366,7 +366,7 @@
>                return nil, nil
>        }
>
> -       header["Transfer-Encoding"] = nil, false
> +       delete(header, "Transfer-Encoding")
>
>        // Head responses have no bodies, so the transfer encoding
>        // should be ignored.
> @@ -399,7 +399,7 @@
>                // Chunked encoding trumps Content-Length. See RFC 2616
>                // Section 4.4. Currently len(te) > 0 implies chunked
>                // encoding.
> -               header["Content-Length"] = nil, false
> +               delete(header, "Content-Length")
>                return te, nil
>        }
>
> Index: src/pkg/http/transport.go
> ===================================================================
> --- a/src/pkg/http/transport.go
> +++ b/src/pkg/http/transport.go
> @@ -266,7 +266,7 @@
>                }
>                if len(pconns) == 1 {
>                        pconn = pconns[0]
> -                       t.idleConn[key] = nil, false
> +                       delete(t.idleConn, key)
>                } else {
>                        // 2 or more cached connections; pop last
>                        // TODO: queue?
> Index: src/pkg/net/fd.go
> ===================================================================
> --- a/src/pkg/net/fd.go
> +++ b/src/pkg/net/fd.go
> @@ -152,7 +152,7 @@
>        if !ok {
>                return nil
>        }
> -       s.pending[key] = nil, false
> +       delete(s.pending, key)
>        return netfd
>  }
>
> @@ -195,7 +195,7 @@
>                }
>                if t > 0 {
>                        if t <= now {
> -                               s.pending[key] = nil, false
> +                               delete(s.pending, key)
>                                if mode == 'r' {
>                                        s.poll.DelFD(fd.sysfd, mode)
>                                        fd.rdeadline = -1
> Index: src/pkg/net/fd_linux.go
> ===================================================================
> --- a/src/pkg/net/fd_linux.go
> +++ b/src/pkg/net/fd_linux.go
> @@ -105,7 +105,7 @@
>                if e := syscall.EpollCtl(p.epfd, syscall.EPOLL_CTL_DEL, fd,
> nil); e != 0 {
>                        print("Epoll delete fd=", fd, ": ",
> os.Errno(e).String(), "\n")
>                }
> -               p.events[fd] = 0, false
> +               delete(p.events, fd)
>        }
>  }
>
> Index: src/pkg/net/textproto/header.go
> ===================================================================
> --- a/src/pkg/net/textproto/header.go
> +++ b/src/pkg/net/textproto/header.go
> @@ -39,5 +39,5 @@
>
>  // Del deletes the values associated with key.
>  func (h MIMEHeader) Del(key string) {
> -       h[CanonicalMIMEHeaderKey(key)] = nil, false
> +       delete(h, CanonicalMIMEHeaderKey(key))
>  }
> Index: src/pkg/net/textproto/pipeline.go
> ===================================================================
> --- a/src/pkg/net/textproto/pipeline.go
> +++ b/src/pkg/net/textproto/pipeline.go
> @@ -108,7 +108,7 @@
>        }
>        c, ok := s.wait[id]
>        if ok {
> -               s.wait[id] = nil, false
> +               delete(s.wait, id)
>        }
>        s.mu.Unlock()
>        if ok {
> Index: src/pkg/old/netchan/export.go
> ===================================================================
> --- a/src/pkg/old/netchan/export.go
> +++ b/src/pkg/old/netchan/export.go
> @@ -314,7 +314,7 @@
>  // delClient forgets the client existed
>  func (exp *Exporter) delClient(client *expClient) {
>        exp.mu.Lock()
> -       exp.clients[client] = false, false
> +       delete(exp.clients, client)
>        exp.mu.Unlock()
>  }
>
> @@ -388,7 +388,7 @@
>        exp.mu.Lock()
>        chDir, ok := exp.names[name]
>        if ok {
> -               exp.names[name] = nil, false
> +               delete(exp.names, name)
>        }
>        // TODO drop all instances of channel from client sets
>        exp.mu.Unlock()
> Index: src/pkg/old/netchan/import.go
> ===================================================================
> --- a/src/pkg/old/netchan/import.go
> +++ b/src/pkg/old/netchan/import.go
> @@ -256,8 +256,8 @@
>        if nc == nil {
>                return os.NewError("netchan import: hangup: no such channel:
> " + name)
>        }
> -       imp.names[name] = nil, false
> -       imp.chans[nc.id] = nil, false
> +       delete(imp.names, name)
> +       delete(imp.chans, nc.id)
>        nc.close()
>        return nil
>  }
> Index: src/pkg/os/inotify/inotify_linux.go
> ===================================================================
> --- a/src/pkg/os/inotify/inotify_linux.go
> +++ b/src/pkg/os/inotify/inotify_linux.go
> @@ -131,7 +131,7 @@
>        if success == -1 {
>                return os.NewSyscallError("inotify_rm_watch", errno)
>        }
> -       w.watches[path] = nil, false
> +       delete(w.watches, path)
>        return nil
>  }
>
> Index: src/pkg/reflect/type.go
> ===================================================================
> --- a/src/pkg/reflect/type.go
> +++ b/src/pkg/reflect/type.go
> @@ -876,7 +876,7 @@
>                fd = inf
>        }
>
> -       mark[t] = false, false
> +       delete(mark, t)
>        return
>  }
>
> Index: src/pkg/rpc/client.go
> ===================================================================
> --- a/src/pkg/rpc/client.go
> +++ b/src/pkg/rpc/client.go
> @@ -104,7 +104,7 @@
>                seq := response.Seq
>                client.mutex.Lock()
>                c := client.pending[seq]
> -               client.pending[seq] = c, false
> +               delete(client.pending, seq)
>                client.mutex.Unlock()
>
>                if response.Error == "" {
> Index: src/pkg/rpc/jsonrpc/client.go
> ===================================================================
> --- a/src/pkg/rpc/jsonrpc/client.go
> +++ b/src/pkg/rpc/jsonrpc/client.go
> @@ -79,7 +79,7 @@
>
>        c.mutex.Lock()
>        r.ServiceMethod = c.pending[c.resp.Id]
> -       c.pending[c.resp.Id] = "", false
> +       delete(c.pending, c.resp.Id)
>        c.mutex.Unlock()
>
>        r.Error = ""
> Index: src/pkg/rpc/jsonrpc/server.go
> ===================================================================
> --- a/src/pkg/rpc/jsonrpc/server.go
> +++ b/src/pkg/rpc/jsonrpc/server.go
> @@ -107,7 +107,7 @@
>                c.mutex.Unlock()
>                return os.NewError("invalid sequence number in response")
>        }
> -       c.pending[r.Seq] = nil, false
> +       delete(c.pending, r.Seq)
>        c.mutex.Unlock()
>
>        if b == nil {
> Index: src/pkg/syscall/syscall_unix.go
> ===================================================================
> --- a/src/pkg/syscall/syscall_unix.go
> +++ b/src/pkg/syscall/syscall_unix.go
> @@ -87,6 +87,6 @@
>        if errno := m.munmap(uintptr(unsafe.Pointer(&b[0])),
> uintptr(len(b))); errno != 0 {
>                return errno
>        }
> -       m.active[p] = nil, false
> +       delete(m.active, p)
>        return 0
>  }
> Index: src/pkg/unicode/maketables.go
> ===================================================================
> --- a/src/pkg/unicode/maketables.go
> +++ b/src/pkg/unicode/maketables.go
> @@ -1152,7 +1152,7 @@
>
>        // Remove class itself.
>        for _, r := range class {
> -               m[r] = false, false
> +               delete(m, r)
>        }
>
>        // What's left is the exceptions.
> Index: src/pkg/unicode/script_test.go
> ===================================================================
> --- a/src/pkg/unicode/script_test.go
> +++ b/src/pkg/unicode/script_test.go
> @@ -206,7 +206,7 @@
>                if !Is(Scripts[test.script], test.rune) {
>                        t.Errorf("IsScript(%U, %s) = false, want true",
> test.rune, test.script)
>                }
> -               notTested[test.script] = false, false
> +               delete(notTested, test.script)
>        }
>        for _, test := range outTest {
>                if Is(Scripts[test.script], test.rune) {
> @@ -230,7 +230,7 @@
>                if !Is(Categories[test.script], test.rune) {
>                        t.Errorf("IsCategory(%U, %s) = false, want true",
> test.rune, test.script)
>                }
> -               notTested[test.script] = false, false
> +               delete(notTested, test.script)
>        }
>        for k := range notTested {
>                t.Error("category not tested:", k)
> @@ -249,7 +249,7 @@
>                if !Is(Properties[test.script], test.rune) {
>                        t.Errorf("IsCategory(%U, %s) = false, want true",
> test.rune, test.script)
>                }
> -               notTested[test.script] = false, false
> +               delete(notTested, test.script)
>        }
>        for k := range notTested {
>                t.Error("property not tested:", k)
> Index: src/pkg/url/url.go
> ===================================================================
> --- a/src/pkg/url/url.go
> +++ b/src/pkg/url/url.go
> @@ -517,7 +517,7 @@
>
>  // Del deletes the values associated with key.
>  func (v Values) Del(key string) {
> -       v[key] = nil, false
> +       delete(v, key)
>  }
>
>  // ParseQuery parses the URL-encoded query string and returns
> Index: src/pkg/xml/xml.go
> ===================================================================
> --- a/src/pkg/xml/xml.go
> +++ b/src/pkg/xml/xml.go
> @@ -390,7 +390,11 @@
>        // translations that were associated with the element we just closed.
>        for p.stk != nil && p.stk.kind != stkStart {
>                s := p.pop()
> -               p.ns[s.name.Local] = s.name.Space, s.ok
> +               if !s.ok {
> +                       delete(p.ns, s.name.Local)
> +               } else {
> +                       p.ns[s.name.Local] = s.name.Space
> +               }
>        }
>
>        return true
> Index: test/nil.go
> ===================================================================
> --- a/test/nil.go
> +++ b/test/nil.go
> @@ -150,7 +150,7 @@
>                m[2] = 3
>        })
>        shouldPanic(func() {
> -               m[2] = 0, false
> +               delete(m, 2)
>        })
>  }
>
>
>
>

[r2, 2011-10-17 20:51:09]

On Oct 17, 2011, at 1:47 PM, Andrew Gerrand wrote:

> Oh wow, I really like how this has turned out.

for me, it's a storm in a teacup. i never understood what all the fuss was
about. at least this may quiet things down. sheesh.

-rob

[cw, 2011-10-17 21:26:50]

Where is the gc change to implement this?

Is it 5265048?  That causes a build break for me.

[rsc, 2011-10-17 22:21:18]

On Mon, Oct 17, 2011 at 17:26,  <cw@f00f.org> wrote:
> Where is the gc change to implement this?
>
> Is it 5265048?  That causes a build break for me.

details?

[cw, 2011-10-18 00:24:15]

On Mon, Oct 17, 2011 at 06:21:16PM -0400, Russ Cox wrote:

> details?

cmd's fail, for example:

cw@lysdexia:~/wk/go/go.hg/src/cmd/goyacc$ make
6l  -o goyacc _go_.6
reflect.(*structType).fieldByNameFunc: undefined: runtime.mapdelete
make: *** [goyacc] Error 1


cw@lysdexia:~/wk/go/go.hg/src/cmd/goyacc$ 6g -V
6g version 10089+


cw@lysdexia:~/wk/go/go.hg/src/cmd/goyacc$ 6l -V
6l version 10089+



src/cmd/gc/runtime.go has:

  +func mapdelete(mapType *byte, hmap map[any]any, key any)

but i don't see any implementation?

missing file?

[rsc, 2011-10-18 11:29:21]

sorry, i will do it again on a clean tree.

[rsc, 2011-10-18 12:48:32]

On Mon, Oct 17, 2011 at 20:24, Chris Wedgwood <cw@f00f.org> wrote:
> but i don't see any implementation?
>
> missing file?

Yes, sorry, the uploaded copy is older than what's on my machine.

Russ

[rsc, 2011-10-18 13:52:30]

Reran gofix -r on a pristine tree and uploaded.
Confirmed that the only manual changes are in
xml/xml.go and exp/types/universe.go.

[rsc, 2011-10-18 13:56:37]

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

gofix -r mapdelete

R=golang-dev, r, adg, r, cw
CC=golang-dev
http://codereview.appspot.com/5266045