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

bytes, strings: Compare tests do not modify input alignments #26129

Open
mundaym opened this issue Jun 29, 2018 · 5 comments
Open

bytes, strings: Compare tests do not modify input alignments #26129

mundaym opened this issue Jun 29, 2018 · 5 comments

Comments

@mundaym
Copy link
Member

@mundaym mundaym commented Jun 29, 2018

I can't see any tests varying the alignment of Compare arguments. Since Compare is written in assembly on most platforms (in internal/bytealg) we should probably be checking that it works even if the inputs are unaligned.

@mundaym mundaym added this to the Unplanned milestone Jun 29, 2018
@LotusFenn
Copy link
Contributor

@LotusFenn LotusFenn commented Jul 6, 2018

I'm interested in working on this. This would require making changes to the bytes/compare_test.go, is that correct? Thanks!

@mundaym
Copy link
Member Author

@mundaym mundaym commented Jul 6, 2018

@LotusFenn Yes, bytes/compare_test.go and strings/compare_test.go. An additional short test in each that just varies the input alignment from, say, 0 to 16 (inclusive) would be great. Thanks!

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 7, 2018

Change https://golang.org/cl/122536 mentions this issue: bytes: vary the input alignment to Compare argument in compare_test.go

gopherbot pushed a commit that referenced this issue Oct 11, 2018
Currently there are no tests that vary the alignment of Compare arguments.
Since Compare is written in assembly on most platforms (in internal/bytealg)
we should be testing different input alignments. This change modifies TestCompare
to vary the alignment of the second argument of Compare.

Updates #26129

Change-Id: I4c30a5adf96a41225df748675f4e9beea413b35c
Reviewed-on: https://go-review.googlesource.com/c/122536
Reviewed-by: Lotus Fenn <fenn.lotus@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@agnivade
Copy link
Contributor

@agnivade agnivade commented Jul 9, 2019

@mundaym - The Compare function in strings does not use assembly and has a simple implementation.

	if a == b {
		return 0
	}
	if a < b {
		return -1
	}
	return +1

It even has a note from @rsc saying -

	// NOTE(rsc): This function does NOT call the runtime cmpstring function,
	// because we do not want to provide any performance justification for
	// using strings.Compare. Basically no one should use strings.Compare.

Would you still want to test unaligned input for strings/compare_test.go ?

@mundaym
Copy link
Member Author

@mundaym mundaym commented Jul 9, 2019

@agnivade Good point. I still think it would be worthwhile though. The compiler could do the substitution with runtime.cmpstring or other alignment-sensitive vector code under the hood (as rsc alluded to in that comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.