Closed Bug 367273 Opened 18 years ago Closed 18 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

Details

Attachments

(1 file, 1 obsolete file)

We should be consistent about #include vs. #import in cocoa widgets.
Attached patch fix v1.0 (obsolete) — Splinter Review
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)
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.
(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.
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?

(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?
Attached patch fix v1.1Splinter Review
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)
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.
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.
Yes, that's just a change I've had in my tree for a while. They aren't needed.
Comment on attachment 251847 [details] [diff] [review]
fix v1.1

r=me
Attachment #251847 - Flags: review+
Attachment #251847 - Flags: review?(stanshebs)
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: