Go: cmd/compile: constant indexing into map literals should be folded

[mdempsky]

This code from internal/syscall/unix/getrandom_linux.go:

package unix
import "runtime"
var randomTrap = map[string]uintptr{
        "386":     355,
        "amd64":   318,
        "arm":     384,
        "ppc64":   359,
        "ppc64le": 359,
}[runtime.GOARCH]

currently compiles into non-trivial chunk of code. However, gc could instead recognize that runtime.GOARCH and all of the map literal keys are constants and that all of the value expressions are side-effect free, and fold to just the corresponding value expression.

[bradfitz]

/cc @randall77

[rsc]

Or people could not write code like that. Come on.

[mdempsky]

It looks like changing the code to:

var randomTrap = func() uintptr {
    switch runtime.GOARCH {
    case "386":
        return 355
    case "amd64":
        return 318
    case "arm":
        return 384
    case "ppc64", "ppc64le":
        return 359
    default:
        return 0
    }
}()

produces simpler code by taking advantage of the existing switch constant folding logic, but still needs a dynamic initializer.

Alternatively, we could just create 4 extra files and use +build rules. (This is my preference if no one's opposed.)

[griesemer]

Is this in any way a performance issue (I can't see why)? If not, what's wrong with the switch?

[mdempsky]

Not a performance issue. Just trying to reduce executable bloat.

Side benefit of using +build rules, it will remind porters to update this file when adding new architectures. E.g., I just realized arm64 isn't using getrandom.

[gopherbot]

CL https://golang.org/cl/16662 mentions this issue.

[davecheney]

@mdempsky I think this is the best approach. There are already per GOOS and GOARCH files for all permutations, and you get to make that value a constant declaration. win/win!

[gopherbot]

CL https://golang.org/cl/20975 mentions this issue.

[rsc]

Indeed. That map thing is a terrible idiom and should not be encouraged.

On Mon, Mar 21, 2016 at 4:01 AM GopherBot [email protected] wrote:

CL https://golang.org/cl/20975 mentions this issue.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/golang/go/issues/10848#issuecomment-199164069

[nightlyone]

One observation while teaching Go: I noticed a lot of people with python as their first and Go now as their second language write this.

Python has no switch statement, so using map (or dict in python) seems ideomatic to them for this use case, when in fact switch is the "table to expression" ideom for those cases in Go.

Any idea where might be a good place to clear that up? Adding an example in the tour comes to mind here.

[qmuntal]

Indeed. That map thing is a terrible idiom and should not be encouraged.

In fact switch is the "table to expression" ideom for those cases in Go

If this is the case, apart of improving the documentation, could we add this case to go vet or even rewrite it using go tool fix? I was also using the map way as an idiomatic construct.

[gopherbot]

Change https://golang.org/cl/231041 mentions this issue: net/http/cgi: replace constant map with switch statement

[ianlancetaylor]

I don't think this rises to the level of a vet check.

[xordspar0]

Is it in scope for a lint check?