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

runtime: merge .go files produced by cgo into their counterparts #12952

Open
nodirt opened this Issue Oct 16, 2015 · 13 comments

Comments

Projects
None yet
6 participants
@nodirt
Member

nodirt commented Oct 16, 2015

Merge

  • panic1.go -> panic.go
  • defs1_darwin.go + defs2_darwin.go -> defs_darwin.go
  • etc

Discussed in https://groups.google.com/forum/#!topic/golang-nuts/yuh2fzooEpM

I'd like to work on this.

@gopherbot

This comment has been minimized.

gopherbot commented Oct 16, 2015

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

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Oct 16, 2015

@gopherbot

This comment has been minimized.

gopherbot commented Oct 16, 2015

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

@gopherbot

This comment has been minimized.

gopherbot commented Oct 16, 2015

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

bradfitz added a commit that referenced this issue Oct 16, 2015

runtime: merge string1.go into string.go
string1.go contents are appended to string.go as is

Updates #12952

Change-Id: I30083ba7fdd362d4421e964a494c76ca865bedc2
Reviewed-on: https://go-review.googlesource.com/15951
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

bradfitz added a commit that referenced this issue Oct 16, 2015

runtime: rename print1.go -> print.go
It seems that it was called print1.go mistakenly: print.go was deleted
in the same commit:
https://go.googlesource.com/go/+/597b266eafe7d63e9be8da1c1b4813bd2998a11c

Updates #12952

Change-Id: I371e59d6cebc8824857df3f3ee89101147dfffc0
Reviewed-on: https://go-review.googlesource.com/15950
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>

bradfitz added a commit that referenced this issue Oct 16, 2015

runtime: merge panic1.go into panic.go
A TODO to merge is removed from panic1.go.
The rest is appended to panic.go

Updates #12952

Change-Id: Ied4382a455abc20bc2938e34d031802e6b4baf8b
Reviewed-on: https://go-review.googlesource.com/15905
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

gopherbot commented Oct 17, 2015

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

bradfitz added a commit that referenced this issue Oct 17, 2015

runtime: merge stack{1,2}.go -> stack.go
* rename stack1.go -> stack.go
* prepend contents of stack2.go to stack.go

Updates #12952

Change-Id: I60d409af37162a5a7596c678dfebc2cea89564ff
Reviewed-on: https://go-review.googlesource.com/16008
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

bradfitz added a commit that referenced this issue Oct 18, 2015

runtime: merge race1.go -> race.go
* append contents of race1.go to race.go
* delete "Implementation of the race detector API." comment
  from race1.go

Updates #12952

Change-Id: Ibdd9c4dc79a63c3bef69eade9525578063c86c1c
Reviewed-on: https://go-review.googlesource.com/16023
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

gopherbot commented Oct 19, 2015

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

bradfitz added a commit that referenced this issue Oct 19, 2015

runtime: merge proc1.go -> proc.go
from proc1.go to proc.go:
* prepend header comment explaining "Goroutine scheduler"
* insert m0 and g0 var defs after the comment
* append the rest

Updates #12952

Change-Id: I35ee9ae3287675cde0c1b6aeaca0a460393f2354
Reviewed-on: https://go-review.googlesource.com/16024
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

gopherbot commented Oct 19, 2015

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

@nodirt

This comment has been minimized.

Member

nodirt commented Oct 20, 2015

See comments in https://go-review.googlesource.com/#/c/16025/

On Tue, Oct 20, 2015, 1:01 AM stemar94 notifications@github.com wrote:

@nodirt https://github.com/nodirt What about runtime{1,2,}.go ?


Reply to this email directly or view it on GitHub
#12952 (comment).

@RLH

This comment has been minimized.

Contributor

RLH commented Oct 20, 2015

In general this will cause a lot of our CLs to have conflicts and slow us
down as we approach the Nov. 1 code freeze. The upside seems minimal given
that any reasonable SDE will figure out where a function is regardless of
the file it is in.

On Tue, Oct 20, 2015 at 8:37 AM, Nodir Turakulov notifications@github.com
wrote:

See comments in https://go-review.googlesource.com/#/c/16025/

On Tue, Oct 20, 2015, 1:01 AM stemar94 notifications@github.com wrote:

@nodirt https://github.com/nodirt What about runtime{1,2,}.go ?


Reply to this email directly or view it on GitHub
#12952 (comment).


Reply to this email directly or view it on GitHub
#12952 (comment).

@nodirt

This comment has been minimized.

Member

nodirt commented Oct 20, 2015

Yes, I was worried that it will interfere with your and @aclements CLs. I can postpone it.

I'm not sure how it slows you down though. https://go-review.googlesource.com/#/c/16008/ demonstrated that git-rebase handles non-invasive changes well (I don't do invasive ones)

@aclements

This comment has been minimized.

Member

aclements commented Oct 20, 2015

It depends on how git sees the merge and rename. Typically, it will rebase changes to the largest hunk of a merged file without trouble, but it will have problems following changes to the smaller hunks over. I'll try to flip through your unmerged CLs later and give my opinion on which should be postponed. I definitely want to see this change and thank you for going to this trouble!

@aclements

This comment has been minimized.

Member

aclements commented Oct 20, 2015

CL 16024 may cause problems, but I see it's already submitted, so we'll just have to see what happens. CL 16025 (runtime*.go) will definitely cause problems, so if you don't mind I'd like to hold off on that one until things have settled down a little (it looks like that one may take a little more thought, anyway). I don't think any of the other CLs will cause any problems.

@nodirt

This comment has been minimized.

Member

nodirt commented Oct 20, 2015

Yes, CL 16025 would cause problems for you. That's why I abandoned it with message "Rearranging runtime_.go is more invasive and requires some planning and coordination with runtime team. I will leave them for now.
It is not the only files I will leave. There is also unaligned_.go that have different build tags."

@mdempsky

This comment has been minimized.

Member

mdempsky commented Oct 23, 2015

I'd like to see the os*_foo.go files collapsed into os_foo.go. Any objections to that?

Rationale: Currently it's rather unfortunate that Gerrit sorts os1_.go files together, then os2_.go files, etc. That makes it more annoying (at least IMO) to review CLs like https://golang.org/cl/16176, where it would be nicer to see the os_foo.go and os1_foo.go changes together.

As far as I can tell, CL 16176 is the only pending CL that touches the os*.go files, but I have a couple more similar cleanups in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment