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: 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 · 8 comments
Milestone

Comments

@gopherbot
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

rsc commented Oct 18, 2013

Comment 2:

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

Status changed to Accepted.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 3:

Labels changed: added repo-main.

@rsc
Copy link
Contributor

rsc commented Mar 3, 2014

Comment 4:

Adding Release=None to all Priority=Someday bugs.

Labels changed: added release-none.

@mdempsky
Copy link
Member

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
@fritzr
Copy link

fritzr commented Aug 18, 2023

A similar issue occurs when one or more fields have a common numeric suffix following the underscore:

struct test {
  int type;
  int pad_1;
  int size;
  int pad_2;
  int etc;
};

Through cgo -godefs:

type Test struct {
        Type    int32
        1       int32 //  syntax error: unexpected literal 1, expected field name or embedded type
        Size    int32
        2       int32 //  syntax error: unexpected literal 2, expected field name or embedded type
        Etc     int32
}

In this example the 'pad_' prefix is stripped and the suffix is not a valid identifier so the output can't compile. Like with the OP's example, it's difficult to recover from this in an automated way since the original field names are lost in the output. It would be nice to at least have a command-line flag to disable the underscore stripping behavior.

@ianlancetaylor
Copy link
Contributor

Do you want to send a patch?

@jclark
Copy link

jclark commented Jan 7, 2024

I was hoping to add support to the unix package for linux/ptp_clock.h, which includes the following struct declarations:

struct ptp_sys_offset {
	unsigned int n_samples; /* Desired number of measurements. */
	unsigned int rsv[3];    /* Reserved for future use. */
	/*
	 * Array of interleaved system/phc time stamps. The kernel
	 * will provide 2*n_samples + 1 time stamps, with the last
	 * one as a system time stamp.
	 */
	struct ptp_clock_time ts[2 * PTP_MAX_SAMPLES + 1];
};

struct ptp_sys_offset_extended {
	unsigned int n_samples; /* Desired number of measurements. */
	unsigned int rsv[3];    /* Reserved for future use. */
	/*
	 * Array of [system, phc, system] time stamps. The kernel will provide
	 * 3*n_samples time stamps.
	 */
	struct ptp_clock_time ts[PTP_MAX_SAMPLES][3];
};

struct ptp_sys_offset_precise {
	struct ptp_clock_time device;
	struct ptp_clock_time sys_realtime;
	struct ptp_clock_time sys_monoraw;
	unsigned int rsv[4];    /* Reserved for future use. */
};

cgo -godefs discards the n_ and sys_ prefixes on the field names, which seems highly unintuitive. Removing the sys_ prefix isn't so bad, but removing the n_ really isn't good.

Code is here

I wonder if we could avoid backwards compatibility problems using a heuristic such as that the prefix will only be removed if the number of fields with the prefix is greater than the number of fields with no prefix (ignoring fields starting with orig_ or _). Or should it be a flag? I am happy to work on a patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants