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: godefs shouldnt try to guess which struct field's identifier prefixes preceding `_` is okay to strip. #5256

Open
gopherbot opened this Issue Apr 10, 2013 · 5 comments

Comments

Projects
None yet
4 participants
@gopherbot

gopherbot commented Apr 10, 2013

by Mortdeus@gocos2d.org:

Godef may produce structs with multiple declarations of the same field identifiers if
its allowed to automatically strip the prefix of a C struct's field identifier. For
example,

When this C struct

struct drm_drawable_info {
    unsigned int num_rects;
        struct drm_clip_rect *rects;
};

is run through godefs it produces the following Go struct.

DrawableInfo struct {
    Rects uint32
    Pad_cgo_0 [4]byte
    Rects     *ClipRect
}

Which obviously isnt correct. 

The problem with this approach is...

A. I have to open the C source code to find out what the old identifier name used to be.
B. Obviously the tool isnt smart enough to know what prefixes are okay and which should
be stripped. 
C. This kind of "magic" implicit functionality is much better implemented as
explicit command line flag with the developer in control.
@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Apr 10, 2013

Comment 1:

The cgo -godefs option is used during the Go bootstrap and is not documented for other
uses.  We aren't going to change its default behaviour.  But I'm fine with changing it
to, e.g., not strip a prefix if it causes duplicate field names.

Labels changed: added priority-later, removed priority-triage.

@rsc

This comment has been minimized.

Contributor

rsc commented Oct 18, 2013

Comment 2:

Labels changed: added priority-someday, removed priority-later.

Status changed to Accepted.

@rsc

This comment has been minimized.

Contributor

rsc commented Dec 4, 2013

Comment 3:

Labels changed: added repo-main.

@rsc

This comment has been minimized.

Contributor

rsc commented Mar 3, 2014

Comment 4:

Adding Release=None to all Priority=Someday bugs.

Labels changed: added release-none.

@mdempsky

This comment has been minimized.

Member

mdempsky commented Aug 13, 2014

Comment 5:

Arguably the bug here is that cgo's "fieldPrefix" function skips over fields that don't
contain a "_" character when trying to come up with a "common" prefix.  E.g., in the
example struct, "rects" does not have an underscore, so fieldPrefix simply skips over
it; then the only field it actually considers is "num_rects" and (erroneously) concludes
"num_" is a common prefix.
It would be easy to tweak fieldPrefix to realize that if a struct has fields without any
underscores then it doesn't have a 'common prefix'.  Unfortunately, that would cause at
least some field names in package syscall to change if they were regenerated; e.g., the
"Unit" field in Sysinfo_t would become "Mem_unit".
However, now that package syscall is frozen (and presumably won't be regenerated any
further), maybe that's not an issue?  Or maybe we still wouldn't want to change the
struct field names even in the new go.sys package?

@rsc rsc added this to the Unplanned milestone Apr 10, 2015

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