code review 7531045: cmd/vet: add assembly checker

cmd/vet: add assembly checker

Fixes issue 5036.

[rsc, 2013-03-22 16:46:28]

Hello golang-dev@googlegroups.com,

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

[r, 2013-03-22 17:00:21]

i don't see the code to associated a.b with a·b in the assembler.

https://codereview.appspot.com/7531045/diff/5001/src/cmd/vet/asmdecl.go
File src/cmd/vet/asmdecl.go (right):

https://codereview.appspot.com/7531045/diff/5001/src/cmd/vet/asmdecl.go#newco...
src/cmd/vet/asmdecl.go:21: // These special kinds are not valid sizes.
weird? it sounds like you're introducing a type but it's just a convention and
is unrelated to the const decl following. it's just a naming convention used far
from here

https://codereview.appspot.com/7531045/diff/5001/src/cmd/vet/asmdecl.go#newco...
src/cmd/vet/asmdecl.go:48: kind  int
here it is!
(is it worth making it a type?)

https://codereview.appspot.com/7531045/diff/5001/src/cmd/vet/asmdecl.go#newco...
src/cmd/vet/asmdecl.go:85: goto HaveAsm
you can do this without a goto easily enough. please don't add one to a program
that doesn't have one already

https://codereview.appspot.com/7531045/diff/5001/src/cmd/vet/test_asm.go
File src/cmd/vet/test_asm.go (right):

https://codereview.appspot.com/7531045/diff/5001/src/cmd/vet/test_asm.go#newc...
src/cmd/vet/test_asm.go:24: func returnnamed(x byte) (r1 int, r2 int16, r3
string, r4 byte)
there are no methods here. i think you need to test a method (because i don't
think the code works for them)

[rsc, 2013-03-22 17:26:38]

PTAL

https://codereview.appspot.com/7531045/diff/5001/src/cmd/vet/asmdecl.go
File src/cmd/vet/asmdecl.go (right):

https://codereview.appspot.com/7531045/diff/5001/src/cmd/vet/asmdecl.go#newco...
src/cmd/vet/asmdecl.go:21: // These special kinds are not valid sizes.
On 2013/03/22 17:00:21, r wrote:
> weird? it sounds like you're introducing a type but it's just a convention and
> is unrelated to the const decl following. it's just a naming convention used
far
> from here

Done.

https://codereview.appspot.com/7531045/diff/5001/src/cmd/vet/asmdecl.go#newco...
src/cmd/vet/asmdecl.go:48: kind  int
On 2013/03/22 17:00:21, r wrote:
> here it is!
> (is it worth making it a type?)

Done.

https://codereview.appspot.com/7531045/diff/5001/src/cmd/vet/asmdecl.go#newco...
src/cmd/vet/asmdecl.go:85: goto HaveAsm
On 2013/03/22 17:00:21, r wrote:
> you can do this without a goto easily enough. please don't add one to a
program
> that doesn't have one already

Done.

[rsc, 2013-03-22 17:27:25]

https://codereview.appspot.com/7531045/diff/5001/src/cmd/vet/test_asm.go
File src/cmd/vet/test_asm.go (right):

https://codereview.appspot.com/7531045/diff/5001/src/cmd/vet/test_asm.go#newc...
src/cmd/vet/test_asm.go:24: func returnnamed(x byte) (r1 int, r2 int16, r3
string, r4 byte)
On 2013/03/22 17:00:21, r wrote:
> there are no methods here. i think you need to test a method (because i don't
> think the code works for them)

It is not possible to implement methods in assembly.

The notation TEXT ·foo(SB) matches func foo here. The regexp looking for TEXT
lines has the · in that case.

[r, 2013-03-22 18:37:59]

LGTM although i suspect this will evolve some.

https://codereview.appspot.com/7531045/diff/11001/src/cmd/vet/asmdecl.go
File src/cmd/vet/asmdecl.go (right):

https://codereview.appspot.com/7531045/diff/11001/src/cmd/vet/asmdecl.go#newc...
src/cmd/vet/asmdecl.go:79: func hasSuffix(pkg *Package, suffix string) bool {
move this to main.go and make it a method of Package. it might be useful to
others.

https://codereview.appspot.com/7531045/diff/11001/src/cmd/vet/asmdecl.go#newc...
src/cmd/vet/asmdecl.go:158: warnf("wrong argument size %d; expected $...-%d",
size, fn.size)
should this be a warn or an error? it's bad.

https://codereview.appspot.com/7531045/diff/11001/src/cmd/vet/asmdecl.go#newc...
src/cmd/vet/asmdecl.go:163: // function, but not an exported one, so stop
checking
why is export relevant to whether we check?

https://codereview.appspot.com/7531045/diff/11001/src/cmd/vet/asmdecl.go#newc...
src/cmd/vet/asmdecl.go:450: if fn.arch.name != "arm" {
i'd prefer you wrote what it is rather than what it isn't - safer that way

[rsc, 2013-03-22 19:07:07]

https://codereview.appspot.com/7531045/diff/11001/src/cmd/vet/asmdecl.go
File src/cmd/vet/asmdecl.go (right):

https://codereview.appspot.com/7531045/diff/11001/src/cmd/vet/asmdecl.go#newc...
src/cmd/vet/asmdecl.go:79: func hasSuffix(pkg *Package, suffix string) bool {
On 2013/03/22 18:37:59, r wrote:
> move this to main.go and make it a method of Package. it might be useful to
> others.

Done.

https://codereview.appspot.com/7531045/diff/11001/src/cmd/vet/asmdecl.go#newc...
src/cmd/vet/asmdecl.go:158: warnf("wrong argument size %d; expected $...-%d",
size, fn.size)
On 2013/03/22 18:37:59, r wrote:
> should this be a warn or an error? it's bad.

They're all bad. In vet error means "print and exit immediately". It's not that
bad.

https://codereview.appspot.com/7531045/diff/11001/src/cmd/vet/asmdecl.go#newc...
src/cmd/vet/asmdecl.go:163: // function, but not an exported one, so stop
checking
On 2013/03/22 18:37:59, r wrote:
> why is export relevant to whether we check?

unfortunate wording. changed to

// function, but not visible from Go (didn't match asmTEXT), so stop checking

https://codereview.appspot.com/7531045/diff/11001/src/cmd/vet/asmdecl.go#newc...
src/cmd/vet/asmdecl.go:450: if fn.arch.name != "arm" {
On 2013/03/22 18:37:59, r wrote:
> i'd prefer you wrote what it is rather than what it isn't - safer that way

Done.

[rsc, 2013-03-22 19:14:47]

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

cmd/vet: add assembly checker

Fixes issue 5036.

R=golang-dev, r
CC=golang-dev
https://codereview.appspot.com/7531045