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.

Sign up for GitHub

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

Jump to bottom

x/sys/unix: consolidate duplicate information #33059

Closed
ianlancetaylor opened this issue Jul 11, 2019 · 8 comments
Closed

x/sys/unix: consolidate duplicate information #33059

ianlancetaylor opened this issue Jul 11, 2019 · 8 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@ianlancetaylor
Copy link
Member

In the current golang.org/x/sys/unix package, there is a lot of duplication, especially in the GNU/Linux files. We currently support 13 different GOARCH values, and more are possible. If I diff, say, zerrors_linux_amd64.go and zerrors_linux_mips.go, I see that a bit more than 500 lines are different, but the files are just over 3000 lines long. That suggests that we are repeating 2500 lines of information 13 times.

This doesn't really matter all that much, but it does make the package somewhat larger than necessary and makes it slightly harder to review CLs. Since we generate the information automatically anyhow, I propose that we add another step: run a merge step on all the files for a given GOOS. For all constants, types, and functions that are defined precisely identically for each GOARCH, move them into a single unified file named simply zerrors_linux.go. Similarly for zsyscall, zsysnum, and ztypes, of course. It should be straightforward to write the merge program based using go/ast. It doesn't have to be smart: just merge definitions that are exactly identical. If in the future the values are different on some specific GOARCH, the automatic generation should automatically leave the definitions back in the GOARCH-specific files.

CC @tklauser @paulzhol @mdlayher @josephlr
@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Jul 11, 2019
@ianlancetaylor ianlancetaylor added this to the Unreleased milestone Jul 11, 2019
@tklauser
Copy link
Member

This sounds like a very good idea to me!

While adding this post-processing step using go/ast we could also port the regexp-based post-processing steps in mkpost.go to use go/ast (as has been proposed multiple times in CL reviews already).

@pierrec
Copy link

pierrec commented Jul 13, 2019

Hello,

I am having a "go" at this, since I have been using the go/ast recently and would like to finally contribute to the project.

I have a few questions as I have started looking into this.

  1. You suggest using .go as the file name holding the factored out consts/types/funcs but there already are such files (e.g. syscall_linux.go exists as well as syscall_linux.go). Or should only the z.go files be considered?

  2. Is preserving the blocks of consts important or could they be all merged?
    Merging them make the implementation (much?) easier.

Thanks.

@ianlancetaylor
Copy link
Member Author

We should only be looking at merging information in the generated files, whose names all start with 'z'. The merged file names should also start with 'z'. We currently have 13 files zerrors_linux_GOARCH.go. I'm suggesting that the common aspects be merged into zerrors_linux.go.

We shouldn't touch the non-generated files, at least not with an automated process.

In the generated files it is not important to preserve blocks of constants.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/221317 mentions this issue: unix: add tool for merging duplicate code

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/221318 mentions this issue: unix: merge duplicate code in linux files

@jupj
Copy link
Contributor

jupj commented Feb 27, 2020

I just sent CL 221318 which merges duplicate code for linux files. To make validation easier, please see https://gist.github.com/jupj/639b5cda305d251161fec2304c27c9b9 for a script, and my results, for comparing the changes in the generated files with the merged files.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/221320 mentions this issue: unix: merge duplicate code in ztypes_linux_*.go

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/221319 mentions this issue: unix: merge duplicate code in zsyscall_linux_*.go

gopherbot pushed a commit to golang/sys that referenced this issue Mar 1, 2020
@jupj @ianlancetaylor
unix: add tool for merging duplicate code
6b2465a 
mkmerge.go parses generated code (z*_GOOS_GOARCH.go) and merges
duplicate consts, funcs, and types, into one file per GOOS (z*_GOOS.go).

Updates golang/go#33059

Change-Id: I1439f260dc8c09e887e5917a3101c39b080f2882
Reviewed-on: https://go-review.googlesource.com/c/sys/+/221317
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit to golang/sys that referenced this issue Mar 1, 2020
@jupj @ianlancetaylor
unix: merge duplicate code in zerrors_linux_*.go
c5d0d7b 
Run mkmerge.go on zerrors_linux_*.go to merge duplicate consts, funcs,
and types into zerrors_linux.go

Please see https://gist.github.com/jupj/639b5cda305d251161fec2304c27c9b9
for a script to validate these changes.

Updates golang/go#33059

Change-Id: I168b5efc7fd6d2fcf7fc8dfe1ef5eea07f07f9c2
Reviewed-on: https://go-review.googlesource.com/c/sys/+/221318
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit to golang/sys that referenced this issue Mar 1, 2020
@jupj @ianlancetaylor
unix: merge duplicate code in zsyscall_linux_*.go
2f85c7e 
Run mkmerge.go on zsyscall_linux_*.go to merge duplicate consts, funcs,
and types into zsyscall_linux.go

Also, run mkmerge.go on zsysnum_linux_*.go; currently there is nothing
to merge.

Please see https://gist.github.com/jupj/639b5cda305d251161fec2304c27c9b9
for a script to validate these changes.

Updates golang/go#33059

Change-Id: I715ada044ee957475669d94b16de059ebb9162d1
Reviewed-on: https://go-review.googlesource.com/c/sys/+/221319
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Mar 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@pierrec @tklauser @jupj @ianlancetaylor @gopherbot