[10.9] plugin_child_interpose.mm: [-Wmissing-declarations] `typedef struct Cursor` declaration does not declare anything

RESOLVED FIXED in mozilla28

Status

()

Core
Plug-ins
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

(Blocks: 1 bug)

Trunk
mozilla28
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 827865 [details] [diff] [review]
Cursor-declaration.patch

When building a 32-bit debug build on OS X 10.9, I see the following warning:

dom/plugins/ipc/interpose/plugin_child_interpose.mm:40:1 [-Wmissing-declarations] `typedef struct Cursor` declaration does not declare anything
Attachment #827865 - Flags: review?(joshmoz)
Did your patch compile?  I don't think it should.

Without "typedef struct Cursor;", how does the compiler know what a "Cursor" object is in the next line?

And even if your patch does compile on Mavericks, I suspect it won't on other versions of OS X.  You might want to do a tryserver run.
Blocks: 894090
Summary: plugin_child_interpose.mm: [-Wmissing-declarations] `typedef struct Cursor` declaration does not declare anything → [10.9] plugin_child_interpose.mm: [-Wmissing-declarations] `typedef struct Cursor` declaration does not declare anything

Updated

5 years ago
Attachment #827865 - Flags: review?(joshmoz) → review?(smichaud)
(Assignee)

Comment 2

5 years ago
Yes, the patch compiles for me. The warning is complaining about combining a typedef with a forward declaration of a struct. My patch replaces the superfluous typedef with a normal forward declaration of `struct Cursor`.
Could you please start a Mac tryserver run for your patch?  "try: -b do -p macosx,macosx64 -u none" should do it.
(Assignee)

Comment 4

5 years ago
Here's a green tryserver build:

https://tbpl.mozilla.org/?tree=Try&rev=a04edd59f7c1
OK, I've now done some tests of my own.  I found that your syntax works in C++ files, but fails (in the way I expected) in C files.

struct Blah;
extern void whatever(Blah blah);

int main(int argc, char *argv[])
{
  return 0;
}

Put this in a file called test.cpp or test.mm, and it compiles just fine with either gcc or clang.

But name the file test.c or test.m, and you get errors like the following:

test.c:2: error: expected ‘)’ before ‘blah’

test.c:2:22: error: must use 'struct' tag to refer to type 'Blah'
extern void whatever(Blah blah);
                     ^
                     struct 

I don't know the C and C++ standards well enough to know why this works in C++ but not in C.  But this behavior is so consistent (I tested gcc back to OS X 10.5) that I don't think it can be just a mistake or a bug.

Given that it's so unexpected, though (at least to an old C hack like me), I think you should add a comment to your patch explaining that your syntax, though legal in C++, is illegal in C.
Comment on attachment 827865 [details] [diff] [review]
Cursor-declaration.patch

Please add a comment explaining that this syntax is legal in C++ but illegal in C.
Attachment #827865 - Flags: review?(smichaud) → review+
> I don't know the C and C++ standards well enough to know why this works in C++ but not in C.

If *you* know them well enough, please cite the specific rule (or rules) here (in this bug) and leave out the comment :-)
(Assignee)

Comment 8

5 years ago
Thanks for double-checking! Both C and C++ support forward declarations of structs like `struct Cursor`, but C require struct types to be referenced using the full name `struct Cursor` like `void SetCursor(struct Cursor* cursor)`.

This code's `typedef struct Cursor` was trying to 1. forward declare `struct Cursor` and 2. typedef struct Cursor to just `Cursor` so C code could just use `void SetCursor(Cursor* cursor)`. If this code was in a header file that need to be compatible with both C and C++ (like the QuickdrawAPI.h header file this code was polyfilling), then a compatible, warning-free change would be `typedef struct Cursor Cursor;`.

Fortunately, this is an Objective-C++ .mm file, we no longer need to worry about C compatibility. <:)
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2ace0287514
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/c2ace0287514
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.