Closed Bug 967006 Opened 6 years ago Closed 6 years ago

clang build failure - stun_build.h:57:9: error: empty struct has size 0 in C, size 1 in C++

Categories

(Core :: WebRTC: Networking, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: alex_y_xu, Assigned: alex_y_xu)

References

(Blocks 1 open bug)

Details

(Whiteboard: [nICEr])

Attachments

(1 file, 2 obsolete files)

34:57.52 In file included from /home/alex/gecko-dev/media/mtransport/nricectx.cpp:75:
34:57.52 In file included from /home/alex/gecko-dev/media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.h:41:
34:57.52 In file included from /home/alex/gecko-dev/media/mtransport/third_party/nICEr/src/stun/stun.h:64:
34:57.52 /home/alex/gecko-dev/media/mtransport/third_party/nICEr/src/stun/stun_build.h:57:9: error: empty struct has size 0 in C, size 1 in C++ [-Werror,-Wextern-c-compat]
34:57.52 typedef struct nr_stun_client_stun_keepalive_params_ {
34:57.52         ^

clang version 3.4 (tags/RELEASE_34/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
This seems like it could be fixed by adding UCHAR dummy;
There is already such code:

typedef struct nr_stun_client_stun_keepalive_params_ {
#ifdef WIN32  // silly VC++ gives error if no members
    int dummy;
#endif
} nr_stun_client_stun_keepalive_params;

I "fix" by replacing with this code:

typedef struct nr_stun_client_stun_keepalive_params_ {
#if defined(WIN32) || defined(__clang__)
    // VC++ and clang give error and warning respectively if no members
    int dummy;
#endif
} nr_stun_client_stun_keepalive_params;

But I'm pretty sure there's a better way to do this.
Blocks: buildwarning
(Note: As a temporary workaround, you should be able to get past this by putting
 ac_add_options --disable-webrtc
in your .mozconfig. (if you don't need WebRTC in your local build.))
Attached patch patch v1 (obsolete) — Splinter Review
working patch attached.

I dunno if we can just delete this struct though, or use #pragma.
Attachment #8372880 - Flags: feedback?(dholbert)
Comment on attachment 8372880 [details] [diff] [review]
patch v1

Thanks - this looks reasonable as a local workaround at least (particularly given that we already suppress a similar error in MSVC).

However, this is in third-party code, and we usually avoid modifying our in-tree copies of third-party code so that our modifications don't just get stomped when we upgrade to the next release of the third-party library.  (instead, we try to fix things upstream and then pull in the fixes, or something along those lines)

Not sure if that's the case for this file or not. It looks like the last thing to touch this file was bug 886120, which did make some substantial modifications to this code in our tree, so we might not be treating this file as delicately as we treat other 3rd-party code.

Hence, I'll optimistically tag :abr (who reviewed bug 886120) for review.
Attachment #8372880 - Flags: review?(adam)
Attachment #8372880 - Flags: feedback?(dholbert)
Attachment #8372880 - Flags: feedback+
Comment on attachment 8372880 [details] [diff] [review]
patch v1

Review of attachment 8372880 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay in getting to this. The solution looks just fine.

In terms of upstreaming this change, EKR and I have broad latitude to upstream stuff into nICEr, since he's the main code author and we both have commit rights to the repo. At the moment, the majority of (and probably only) development being done on this library is in the Mozilla tree. When we get a breather, the plan is to bundle all our improvements together and push them back upstream.
Attachment #8372880 - Flags: review?(adam) → review+
Whiteboard: [nICEr]
Alex, could you re-post with author info / commit message included in the headers, as described here:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Then, you can mark this as "checkin-needed" and someone can import & push your updated patch.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → alex_y_xu
Flags: needinfo?(alex_y_xu)
Attached patch patch v1 hg format (obsolete) — Splinter Review
Attachment #8372880 - Attachment is obsolete: true
Flags: needinfo?(alex_y_xu)
Keywords: checkin-needed
apparently git-patch-to-hg-format outputs peculiar commit messages.
Attachment #8380114 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/195910dc7604
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.