Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: switch to $ placeholder for function names in asm_test.go #21500

Closed
ALTree opened this issue Aug 17, 2017 · 3 comments

Comments

Projects
None yet
3 participants
@ALTree
Copy link
Member

commented Aug 17, 2017

I found that adding new tests in cmd/compile/internal/gc/asm_test.go can be slightly annoying because all the test functions for a given architecture are dumped in the same file, so the function names need to be unique.

Most of the names go like f0, f1, ... f80. They are not informative and if you need, for example, to add a variant of test f17, you can:

  • call it f81 and put it after f17 (making the list unsorted);
  • call it f81 and put it at the end (now f17 and f81, which test the same thing, are far away);
  • add some suffix like f17_2 (names are already ugly, this will make it worse).

This is not a problem for architectures like 386, which has like 4 tests, but amd64 has ~100 tests and at least 70 of them are named f%d, which also makes pretty clear that function names are not being used to explain what the function is testing (it seems that people prefer to put a sentence in a comment above the test struct).

I propose switching to a name-placeholder $ in the test functions, like this:

	{
		`
		func $(x uint64) uint64 {
			return x<<7 + x>>57
		}
		`,
		[]string{"\tROLQ\t[$]7,"},
	},

and modify the function that dumps the tests on file to replace $ with func_X with X = 0, 1, 2 ...

The only drawback is slightly worse failure messages. Right now they say:

--- FAIL: TestAssembly/platform/linux/386 (0.52s)
      asm_test.go:87: expected:	IMULQ	[$]23

    go:
        func f1(n int) int {
            return 9*n + 14*n
       }

and you can search for f1 in the test file, but it's not foul-proof because there are half a dozen of test functions called f1 in the file (one in the amd64 array, another in the arm array, etc...).

After the change they would say:

--- FAIL: TestAssembly/platform/linux/386 (0.52s)
      asm_test.go:87: expected:	IMULQ	[$]23

    go:
        func func_1(n int) int {      <---------  different
            return 9*n + 14*n
       }

and the func_1 name is auto-generated, so it's not in the file. You can still search for the 9*n part, tough (we always print the func body); and this is only a problem when there's a test failure.

Opinions? @randall77 @josharian @TocarIP

@randall77

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2017

Sounds fine to me. Maybe make the name indicate location? A line number would be ideal, but maybe not feasible. An arch + index into the test array would be acceptable.

@ALTree

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2017

Ok, nice. Will look into this and send a patch if the end result is convincing.

@ALTree ALTree self-assigned this Aug 18, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Aug 22, 2017

Change https://golang.org/cl/57292 mentions this issue: cmd/compile: support placeholder name '$' in code generation tests

@gopherbot gopherbot closed this in 8bca7ef Aug 22, 2017

@golang golang locked and limited conversation to collaborators Aug 22, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.