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

all: simplify s[:] to s where s is a slice #267

Closed
wants to merge 1 commit into from

Conversation

ludweeg
Copy link

@ludweeg ludweeg commented Oct 4, 2018

No description provided.

@beyang
Copy link
Member

beyang commented Oct 5, 2018

I believe the purpose of the [:] syntax in these instances is to create a shallow copy of the slice as noted here: https://github.com/sourcegraph/sourcegraph/pull/267/files#diff-59c7a605549b380967da6d1b158a7d4cL240. That behavior is important so I'm going to close this PR.

@quasilyte
Copy link
Contributor

This is not true as returning a slice in

	return c.messages

Would create a shallow copy of c.messages. Go always does a "shallow copy" for values and arguments, this is its semantics documented by the spec.
In case of slices, it's a 24-byte copy on AMD64 machine.

Actually, compiler optimizes s[:][:] to s (and it optimizes s[:] to s as well).

@quasilyte
Copy link
Contributor

Given this code:

package example

func f2(xs []int) []int {
	return xs[:][:]
}

func f1(xs []int) []int {
	return xs[:]
}

func f0(xs []int) []int {
	return xs
}

The output is:

"".f2 STEXT nosplit size=31 args=0x30 locals=0x0
	0x0000 00000 (foo.go:3)	TEXT	"".f2(SB), NOSPLIT, $0-48
	0x0000 00000 (foo.go:3)	FUNCDATA	$0, gclocals·74143377807d4351509ac14898b0a46e(SB)
	0x0000 00000 (foo.go:3)	FUNCDATA	$1, gclocals·7d2d5fca80364273fb07d5820a76fef4(SB)
	0x0000 00000 (foo.go:3)	FUNCDATA	$3, gclocals·9fb7f0986f647f17cb53dda1484e0f7a(SB)
	0x0000 00000 (foo.go:4)	PCDATA	$2, $1
	0x0000 00000 (foo.go:4)	PCDATA	$0, $1
	0x0000 00000 (foo.go:4)	MOVQ	"".xs+8(SP), AX
	0x0005 00005 (foo.go:4)	PCDATA	$2, $0
	0x0005 00005 (foo.go:4)	MOVQ	AX, "".~r1+32(SP)
	0x000a 00010 (foo.go:4)	MOVQ	"".xs+16(SP), AX
	0x000f 00015 (foo.go:4)	MOVQ	AX, "".~r1+40(SP)
	0x0014 00020 (foo.go:4)	PCDATA	$0, $2
	0x0014 00020 (foo.go:4)	MOVQ	"".xs+24(SP), AX
	0x0019 00025 (foo.go:4)	MOVQ	AX, "".~r1+48(SP)
	0x001e 00030 (foo.go:4)	RET
	0x0000 48 8b 44 24 08 48 89 44 24 20 48 8b 44 24 10 48  H.D$.H.D$ H.D$.H
	0x0010 89 44 24 28 48 8b 44 24 18 48 89 44 24 30 c3     .D$(H.D$.H.D$0.
"".f1 STEXT nosplit size=31 args=0x30 locals=0x0
	0x0000 00000 (foo.go:7)	TEXT	"".f1(SB), NOSPLIT, $0-48
	0x0000 00000 (foo.go:7)	FUNCDATA	$0, gclocals·74143377807d4351509ac14898b0a46e(SB)
	0x0000 00000 (foo.go:7)	FUNCDATA	$1, gclocals·7d2d5fca80364273fb07d5820a76fef4(SB)
	0x0000 00000 (foo.go:7)	FUNCDATA	$3, gclocals·9fb7f0986f647f17cb53dda1484e0f7a(SB)
	0x0000 00000 (foo.go:8)	PCDATA	$2, $1
	0x0000 00000 (foo.go:8)	PCDATA	$0, $1
	0x0000 00000 (foo.go:8)	MOVQ	"".xs+8(SP), AX
	0x0005 00005 (foo.go:8)	PCDATA	$2, $0
	0x0005 00005 (foo.go:8)	MOVQ	AX, "".~r1+32(SP)
	0x000a 00010 (foo.go:8)	MOVQ	"".xs+16(SP), AX
	0x000f 00015 (foo.go:8)	MOVQ	AX, "".~r1+40(SP)
	0x0014 00020 (foo.go:8)	PCDATA	$0, $2
	0x0014 00020 (foo.go:8)	MOVQ	"".xs+24(SP), AX
	0x0019 00025 (foo.go:8)	MOVQ	AX, "".~r1+48(SP)
	0x001e 00030 (foo.go:8)	RET
	0x0000 48 8b 44 24 08 48 89 44 24 20 48 8b 44 24 10 48  H.D$.H.D$ H.D$.H
	0x0010 89 44 24 28 48 8b 44 24 18 48 89 44 24 30 c3     .D$(H.D$.H.D$0.
"".f0 STEXT nosplit size=31 args=0x30 locals=0x0
	0x0000 00000 (foo.go:11)	TEXT	"".f0(SB), NOSPLIT, $0-48
	0x0000 00000 (foo.go:11)	FUNCDATA	$0, gclocals·74143377807d4351509ac14898b0a46e(SB)
	0x0000 00000 (foo.go:11)	FUNCDATA	$1, gclocals·7d2d5fca80364273fb07d5820a76fef4(SB)
	0x0000 00000 (foo.go:11)	FUNCDATA	$3, gclocals·9fb7f0986f647f17cb53dda1484e0f7a(SB)
	0x0000 00000 (foo.go:12)	PCDATA	$2, $1
	0x0000 00000 (foo.go:12)	PCDATA	$0, $1
	0x0000 00000 (foo.go:12)	MOVQ	"".xs+8(SP), AX
	0x0005 00005 (foo.go:12)	PCDATA	$2, $0
	0x0005 00005 (foo.go:12)	MOVQ	AX, "".~r1+32(SP)
	0x000a 00010 (foo.go:12)	MOVQ	"".xs+16(SP), AX
	0x000f 00015 (foo.go:12)	MOVQ	AX, "".~r1+40(SP)
	0x0014 00020 (foo.go:12)	PCDATA	$0, $2
	0x0014 00020 (foo.go:12)	MOVQ	"".xs+24(SP), AX
	0x0019 00025 (foo.go:12)	MOVQ	AX, "".~r1+48(SP)
	0x001e 00030 (foo.go:12)	RET
	0x0000 48 8b 44 24 08 48 89 44 24 20 48 8b 44 24 10 48  H.D$.H.D$ H.D$.H
	0x0010 89 44 24 28 48 8b 44 24 18 48 89 44 24 30 c3     .D$(H.D$.H.D$0.
go.loc."".f2 SDWARFLOC size=0
go.info."".f2 SDWARFINFO size=54

@keegancsmith
Copy link
Member

@quasilyte is correct, [:] does nothing in all the cases listed. A trick we may want to use in some of those cases is x[::len(x)] so that if we append, we copy the underlying data. Given we are wanting a "shallow copy" that is probably what we want to do since the previous user assumed random writes would be bad due to "shallow" (I hope). We should probably abandon this PR and actually validate each line here to see if we need to copy, limit cap or can just return the slide.

chrismwendt pushed a commit that referenced this pull request Nov 4, 2018
This commit fixes access token usage by:

- Not setting the `authorization` header when
requesting from the options page.
Chrome suddenly becan rejecting the request in
pre-flight checks from hte options page.
- _Only_ creating access tokens when the server connection is checked in the
options page.
- Save the access token ID so we can ensure it still exists on the
server.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants