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: copy parameter names to exported C file where possible #37746

Closed
nathan-fiscaletti opened this issue Mar 8, 2020 · 5 comments
Closed
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@nathan-fiscaletti
Copy link
Contributor

nathan-fiscaletti commented Mar 8, 2020

The Problem

According to this cgo documentation, when exporting functions using cgo, the resulting header file should retain your function parameters names, however it practice it does not.

It States

Go functions can be exported for use by C code in the following way:

//export MyFunction
func MyFunction(arg1, arg2 int, arg3 string) int64 {...}

//export MyFunction2
func MyFunction2(arg1, arg2 int, arg3 string) (int64, *C.char) {...}

They will be available in the C code as:

extern int64 MyFunction(int arg1, int arg2, GoString arg3);
extern struct MyFunction2_return MyFunction2(int arg1, int arg2, GoString arg3);

However, when I compile that exact code it gave, i get this result:

extern GoInt64 MyFunction(GoInt p0, GoInt p1, GoString p2);
extern struct MyFunction2_return MyFunction2(GoInt p0, GoInt p1, GoString p2);

While this documentation that i've linked is wrong both in the types it assigns to the parameters and their return types and the way that their names are exported (should probably be updated), the documentation isn't really the focus of my proposal.

Based on this stack overflow thread I had posted a while back, it seems that the reason for this is that "Go allows arbitrary rune names, e.g., instead of input you might call the variable π. C does not allow such names."

My Proposal

Instead of avoiding this using parameter names such as p0, p1, p2, etc, we first detect if the function name is using a non-ASCII character in its name using something like this (taken from this SO thread)

func isASCII(s string) bool {
    for i := 0; i < len(s); i++ {
        if s[i] > unicode.MaxASCII {
            return false
        }
    }
    return true
}

And then we could do one of several things.

  1. Export all non-ASCII parameters as they are now using p0, p1, etc, but allow parameter names that are proper ASCII to use their original names.
  2. Remove the non-ASCII characters from the parameter name and print a warning during compilation.
  3. Do not allow parameter names that include non-ASCII characters in C exported functions, but allow them in regular go functions. (this is my preference)
  4. I'm open to other solutions?

This normally wouldn't be an issue, but when a functions documentation block doesn't line up properly with the parameter names for the function, you wind up with incorrect documentation in either the source file or in the exported header, and it interferes with consumption of the exported header file.

Here is a basic example that somewhat demonstrates how the documentation can become misaligned.

/**
 * Parses the name based on the type.
 *
 * @param type the type
 * @param name the name
 */
//export ParseValue
func ParseValue(type C.int, name *C.char) {
    // do something with the data
}

results in

/**
 * Parses the name based on the type.
 *
 * @param type the type
 * @param name the name
 */
extern void ParseValue(int p0, char* p1);

Consuming that function externally might not be incredibly difficult if you infer the position of the parameters, but when you have functions with much more complex signatures or functions with parameter documentation out of order, things get hairy and it tends to result in you maintaining documentation that does not line up either in source or does not line up in implementation.

I personally think the best solution would be to remove the allowance of runes in functions that are exported through cgo specifically. This way function parameters will line up in the C header and the Go source file and add a much needed congruity.

@gopherbot gopherbot added this to the Proposal milestone Mar 8, 2020
@ianlancetaylor
Copy link
Contributor

Thanks for filing the issue. This doesn't need to be a proposal. If someone wants to modify cgo as you suggest, that would be fine.

@ianlancetaylor ianlancetaylor changed the title proposal: a way to give cgo exported function parameters proper names cmd/cgo: copy parameter names to exported C file where possible Mar 8, 2020
@ianlancetaylor ianlancetaylor added NeedsFix The path to resolution is known, but the work has not been done. and removed Proposal labels Mar 8, 2020
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Backlog Mar 8, 2020
@gopherbot
Copy link

Change https://golang.org/cl/222619 mentions this issue: cmd/cgo: updated exported function parameter names

@nathan-fiscaletti
Copy link
Contributor Author

nathan-fiscaletti commented Mar 9, 2020

Submitted a PR for this and tested with the code in the official documentation. Seems to work now.

extern GoInt64 MyFunction(GoInt arg1, GoInt arg2, GoString arg3);

/* Return type for MyFunction2 */
struct MyFunction2_return {
	GoInt64 r0;
	char* r1;
};
extern struct MyFunction2_return MyFunction2(GoInt arg1, GoInt arg2, GoString arg3);

@gopherbot
Copy link

Change https://golang.org/cl/222622 mentions this issue: cmd/cgo: always produce a parameter name for C code

gopherbot pushed a commit that referenced this issue Mar 10, 2020
Updates #37746

Change-Id: Ib64abe3995f310cd50ede47b0d3d159572901000
Reviewed-on: https://go-review.googlesource.com/c/go/+/222622
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Mar 10, 2021
@ianlancetaylor
Copy link
Contributor

This seems to have caused #44974.

@dmitshur dmitshur modified the milestones: Backlog, Go1.15 Mar 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants