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/cgo: updated exported function parameter names #37750

Closed
wants to merge 5 commits into from

Conversation

@nathan-fiscaletti
Copy link
Contributor

nathan-fiscaletti commented Mar 9, 2020

Functions that are exported through cgo will now retain their
parameter names provided that their parameter names use only
ASCII characters.

Fixes #37746

functions exported through cgo will now require that parameter names use
only ASCII characters. this way, the declarations in the exported header
file will retain the proper names for parameters

Fixes: #37746
@googlebot googlebot added the cla: yes label Mar 9, 2020
@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

This PR (HEAD: b7da1b5) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/222619 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

Message from Ian Lance Taylor:

Patch Set 1:

(2 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

…y have only ASCII characters but with p0, p1, etc if they have ASCII characters
@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

This PR (HEAD: 11b5856) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/222619 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

Message from Nathan Fiscaletti:

Patch Set 3:

(2 comments)

👍


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

Message from Ian Lance Taylor:

Patch Set 3:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

This PR (HEAD: c39651d) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/222619 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

Message from Nathan Fiscaletti:

Patch Set 4:

(1 comment)

Let me know if i should change the name or the documentation of the function, only parts I'm not certain are good enough.


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

Message from Ian Lance Taylor:

Patch Set 4:

(3 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

…documentation for the function
@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

This PR (HEAD: e9d7225) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/222619 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

Message from Nathan Fiscaletti:

Patch Set 6:

(3 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

Message from Ian Lance Taylor:

Patch Set 7: Run-TryBot+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

Message from Gobot Gobot:

Patch Set 7:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=a0438ecf


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

Message from Gobot Gobot:

Patch Set 7:

Build is still in progress...
This change failed on freebsd-amd64-12_0:
See https://storage.googleapis.com/go-build-log/a0438ecf/freebsd-amd64-12_0_02a7184b.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

Message from Gobot Gobot:

Patch Set 7: TryBot-Result-1

6 of 20 TryBots failed:
Failed on freebsd-amd64-12_0: https://storage.googleapis.com/go-build-log/a0438ecf/freebsd-amd64-12_0_02a7184b.log
Failed on openbsd-amd64-64: https://storage.googleapis.com/go-build-log/a0438ecf/openbsd-amd64-64_ba786553.log
Failed on linux-386: https://storage.googleapis.com/go-build-log/a0438ecf/linux-386_911e6ff3.log
Failed on linux-amd64: https://storage.googleapis.com/go-build-log/a0438ecf/linux-amd64_c398fcef.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/a0438ecf/windows-amd64-2016_9cd19abc.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/a0438ecf/windows-386-2008_f7a0e454.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

Message from Nathan Fiscaletti:

Patch Set 7:

Patch Set 7: TryBot-Result-1

6 of 20 TryBots failed:
Failed on freebsd-amd64-12_0: https://storage.googleapis.com/go-build-log/a0438ecf/freebsd-amd64-12_0_02a7184b.log
Failed on openbsd-amd64-64: https://storage.googleapis.com/go-build-log/a0438ecf/openbsd-amd64-64_ba786553.log
Failed on linux-386: https://storage.googleapis.com/go-build-log/a0438ecf/linux-386_911e6ff3.log
Failed on linux-amd64: https://storage.googleapis.com/go-build-log/a0438ecf/linux-amd64_c398fcef.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/a0438ecf/windows-amd64-2016_9cd19abc.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/a0438ecf/windows-386-2008_f7a0e454.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Are there build artifacts from running tests that I can see to potentially resolve this? I'd like to see the intermediate _cgo_export.c that's being exported from ./misc/cgo/life/testdata, maybe i can resolve this. I had a feeling that there would be something that came up from changing this part of cgo.


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

Message from Nathan Fiscaletti:

Patch Set 7:

Patch Set 7:

Patch Set 7: TryBot-Result-1

6 of 20 TryBots failed:
Failed on freebsd-amd64-12_0: https://storage.googleapis.com/go-build-log/a0438ecf/freebsd-amd64-12_0_02a7184b.log
Failed on openbsd-amd64-64: https://storage.googleapis.com/go-build-log/a0438ecf/openbsd-amd64-64_ba786553.log
Failed on linux-386: https://storage.googleapis.com/go-build-log/a0438ecf/linux-386_911e6ff3.log
Failed on linux-amd64: https://storage.googleapis.com/go-build-log/a0438ecf/linux-amd64_c398fcef.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/a0438ecf/windows-amd64-2016_9cd19abc.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/a0438ecf/windows-386-2008_f7a0e454.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Are there build artifacts from running tests that I can see to potentially resolve this? I'd like to see the intermediate _cgo_export.c that's being exported from ./misc/cgo/life/testdata, maybe i can resolve this. I had a feeling that there would be something that came up from changing this part of cgo.

This does (to me at least) look like an oversight in that test that was written


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

Message from Ian Lance Taylor:

Patch Set 7:

Are there build artifacts from running tests that I can see to potentially resolve this? I'd like to see the intermediate _cgo_export.c that's being exported from ./misc/cgo/life/testdata, maybe i can resolve this. I had a feeling that there would be something that came up from changing this part of cgo.

This does (to me at least) look like an oversight in that test that was written

If you run the test with go test -work, the build artifacts will be left in a /tmp subdirectory. You have to poke around a bit, or also use the -x option to see exactly where the files are written.

Here it looks like the local variable needs to be renamed. The similar code in writeOutputFunc uses _cgo_a, and probably writeExports should do the same.


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

Message from Nathan Fiscaletti:

Patch Set 7:

Patch Set 7:

Are there build artifacts from running tests that I can see to potentially resolve this? I'd like to see the intermediate _cgo_export.c that's being exported from ./misc/cgo/life/testdata, maybe i can resolve this. I had a feeling that there would be something that came up from changing this part of cgo.

This does (to me at least) look like an oversight in that test that was written

If you run the test with go test -work, the build artifacts will be left in a /tmp subdirectory. You have to poke around a bit, or also use the -x option to see exactly where the files are written.

Here it looks like the local variable needs to be renamed. The similar code in writeOutputFunc uses _cgo_a, and probably writeExports should do the same.

Is there an easy way to run the same test that was run in this trybot? Just so i can make sure things are working locally before i push a resolution.


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2020

Message from Nathan Fiscaletti:

Patch Set 7:

Patch Set 7:

Are there build artifacts from running tests that I can see to potentially resolve this? I'd like to see the intermediate _cgo_export.c that's being exported from ./misc/cgo/life/testdata, maybe i can resolve this. I had a feeling that there would be something that came up from changing this part of cgo.

This does (to me at least) look like an oversight in that test that was written

If you run the test with go test -work, the build artifacts will be left in a /tmp subdirectory. You have to poke around a bit, or also use the -x option to see exactly where the files are written.

Here it looks like the local variable needs to be renamed. The similar code in writeOutputFunc uses _cgo_a, and probably writeExports should do the same.

Strangely enough, the same test seems to succeed locally for me?

$ cd misc/cgo/life
$ go test -work
WORK=/var/folders/w3/_t_vv2jd6tb1hvj_m7jffg940000gn/T/go-build318113539
PASS
ok  	misc/cgo/life	2.610s

Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 10, 2020

This PR (HEAD: 220959b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/222619 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 10, 2020

Message from Nathan Fiscaletti:

Patch Set 8:

Patch Set 7: TryBot-Result-1

6 of 20 TryBots failed:
Failed on freebsd-amd64-12_0: https://storage.googleapis.com/go-build-log/a0438ecf/freebsd-amd64-12_0_02a7184b.log
Failed on openbsd-amd64-64: https://storage.googleapis.com/go-build-log/a0438ecf/openbsd-amd64-64_ba786553.log
Failed on linux-386: https://storage.googleapis.com/go-build-log/a0438ecf/linux-386_911e6ff3.log
Failed on linux-amd64: https://storage.googleapis.com/go-build-log/a0438ecf/linux-amd64_c398fcef.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/a0438ecf/windows-amd64-2016_9cd19abc.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/a0438ecf/windows-386-2008_f7a0e454.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

I believe this is resolved with patch set 8


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 10, 2020

Message from Ian Lance Taylor:

Patch Set 8: Run-TryBot+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 10, 2020

Message from Gobot Gobot:

Patch Set 8:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=7b1b699d


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 10, 2020

Message from Nathan Fiscaletti:

Patch Set 8:

Patch Set 8:

Patch Set 7: TryBot-Result-1

6 of 20 TryBots failed:
Failed on freebsd-amd64-12_0: https://storage.googleapis.com/go-build-log/a0438ecf/freebsd-amd64-12_0_02a7184b.log
Failed on openbsd-amd64-64: https://storage.googleapis.com/go-build-log/a0438ecf/openbsd-amd64-64_ba786553.log
Failed on linux-386: https://storage.googleapis.com/go-build-log/a0438ecf/linux-386_911e6ff3.log
Failed on linux-amd64: https://storage.googleapis.com/go-build-log/a0438ecf/linux-amd64_c398fcef.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/a0438ecf/windows-amd64-2016_9cd19abc.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/a0438ecf/windows-386-2008_f7a0e454.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

I believe this is resolved with patch set 8

I was unable to get the test to run on my linux/amd64 vm, but it ran successfully on my darwin/amd64 system.


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 10, 2020

Message from Gobot Gobot:

Patch Set 8: TryBot-Result+1

TryBots are happy.


Please don’t reply on this GitHub thread. Visit golang.org/cl/222619.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request Mar 10, 2020
Functions that are exported through cgo will now retain their
parameter names provided that their parameter names use only
ASCII characters.

Fixes #37746

Change-Id: Ia5f643e7d872312e81c224febd1f81ce14425c32
GitHub-Last-Rev: 220959b
GitHub-Pull-Request: #37750
Reviewed-on: https://go-review.googlesource.com/c/go/+/222619
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 10, 2020

This PR is being closed because golang.org/cl/222619 has been merged.

@gopherbot gopherbot closed this Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.