All users were logged out of Bugzilla on October 13th, 2018

be consistent about #include vs. #import in cocoa widgets

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: jaas, Assigned: jaas)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

51.83 KB, patch
nick.kreeger
: review+
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
We should be consistent about #include vs. #import in cocoa widgets.
(Assignee)

Comment 1

12 years ago
Created attachment 251815 [details] [diff] [review]
fix v1.0

This brings all of cocoa widgets into compliance with the following rule, which I hammered out with Stan Shebs: "use #import for Apple frameworks, #include for everything else."
Attachment #251815 - Flags: review?(stanshebs)

Comment 2

12 years ago
You're not going to be able to pull this off *that* easily! :-) Gotta make sure that included moz files have include guards - I don't see any on mozAccessibleProtocol.h, nsMacCursor.h, etc.

In nsCursorManager.mm, that should be <math.h> - presumably, I don't see a math.h in the source tree. (Yes, #include "foo.h" does look in system places if file is not found in source places.)

Looks right otherwise, will be a nice cleanup.

Comment 3

12 years ago
(In reply to comment #2)
> You're not going to be able to pull this off *that* easily! :-) Gotta make sure
> that included moz files have include guards - I don't see any on
> mozAccessibleProtocol.h, nsMacCursor.h, etc.

AFAIK, the whole point of #import is that it ensures that a header will only ever be included once, so you don't have to add include guards.

How about the rule "only use #import for objective-c headers"? I don't see why you'd ever use #include for objc.

Comment 4

12 years ago
Yeah, with #import the lack of include guards was reasonable.

I'm prejudiced against #import, perhaps because of unfortunate experiences trying to get it right in GCC (How do you know if two headers on a system are "the same" without opening and reading them?), so I tend to only want to use it if necessary. What is an "objective-C header?" Is it still one when I've removed all the @-constructs, perhaps to make them more private, but left extern C decls?

Comment 5

12 years ago
(In reply to comment #4)
> What is an "objective-C header?" Is it still one when I've
> removed all the @-constructs, perhaps to make them more private, but left
> extern C decls?

If you have no objective-c in it, then obviously it's not an objective-c header. An objc file will contain objc code, and only other objc(++) files can include them, so what's the issue with #import?
(Assignee)

Comment 6

12 years ago
Created attachment 251847 [details] [diff] [review]
fix v1.1

This also standardizes header guards for all headers in cocoa widgets. Also includes some cairo macro cleanups (we don't need to ifdef for it any more).
Attachment #251815 - Attachment is obsolete: true
Attachment #251847 - Flags: review?(stanshebs)
Attachment #251815 - Flags: review?(stanshebs)

Comment 7

12 years ago
If you change an objc header to not have objc, then the suggested rule requires changing all the clients to use #include instead of #import. I think a useful rule for headers is that you shouldn't have to change how they're included no matter what the contents - Apple's most heavily-engineered headers are conditionalized so they can be consumed by any dialect of C, and some non-C compilers (like Rez). But I've had to argue (both sides of, ahem) the #import/#include argument a hundred times, and there are good arguments either way. What really swings it for me in Moz code is that we have small amounts of Mac-specific code scattered in a great big mass of non-Mac code, and so if there are no serious penalties for doing so, our best strategy overall is to keep Mac-only constructs to the minimum possible.

Comment 8

12 years ago
The patch looks great, you caught a lot of inconsistencies! (Grepping shows up to three underscores after the _h in some other parts of the sources ...)  I assume you empirically determined that the two headers dropped from nsNativeThemeCocoa.mm weren't actually needed.
(Assignee)

Comment 9

12 years ago
Yes, that's just a change I've had in my tree for a while. They aren't needed.

Comment 10

12 years ago
Comment on attachment 251847 [details] [diff] [review]
fix v1.1

r=me
Attachment #251847 - Flags: review+
(Assignee)

Updated

12 years ago
Attachment #251847 - Flags: review?(stanshebs)
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.