Summary: | Clean up: Move XPC forward declarations in JavaScriptCore to WTF SPI wrapper header | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||
Component: | Web Template Framework | Assignee: | Daniel Bates <dbates> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aestes, ap, benjamin, cmarcelo, commit-queue, ddkilzer, ggaren, joepeck, mjs | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | iPhone / iPad | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 137320 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Daniel Bates
2014-09-30 17:11:34 PDT
Created attachment 238978 [details]
Patch
Attachment 238978 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/spi/cocoa/XPCSPI.h:38: xpc_connection_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/XPCSPI.h:50: xpc_object_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/XPCSPI.h:51: xpc_connection_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/XPCSPI.h:57: xpc_type_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Total errors found: 4 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 239035 [details]
Patch
_xpc_error_connection_invalid and _xpc_error_termination_imminent are only const in OS X >= 10.9
Attachment 239035 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/spi/cocoa/XPCSPI.h:38: xpc_connection_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/XPCSPI.h:50: xpc_object_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/XPCSPI.h:51: xpc_connection_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/XPCSPI.h:57: xpc_type_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Total errors found: 4 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 239035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239035&action=review > Source/WTF/ChangeLog:14 > + * wtf/spi/cocoa/XPCSPI.h: Added. It is annoying to have this in a cocoa directory, because XPC has nothing to do with Cocoa. Maybe darwin? > Source/WTF/wtf/spi/cocoa/XPCSPI.h:87 > +EXTERN_C CONST_ON_OR_AFTER_MAC_OS_X_VERSION_1090 struct _xpc_dictionary_s _xpc_error_connection_invalid; Where does EXTERN_C come from? I'd prefer a plain extern "C" - it's no more verbose, but clearly less fragile. > Source/WTF/wtf/spi/cocoa/XPCSPI.h:99 > +#if __BLOCKS__ We use "#ifdef __BLOCKS__" elsewhere in WebKit. Which is right? > Source/WTF/wtf/spi/cocoa/XPCSPI.h:152 > +#if !defined(xpc_retain) && OS_OBJECT_USE_OBJC_RETAIN_RELEASE > +#define xpc_retain(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o retain]; }) > +#endif > + > +#if !defined(xpc_release) && OS_OBJECT_USE_OBJC_RETAIN_RELEASE > +#define xpc_release(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o release]; }) > +#endif Does this compile in .cpp files? (In reply to comment #5) > (From update of attachment 239035 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=239035&action=review > > > Source/WTF/ChangeLog:14 > > + * wtf/spi/cocoa/XPCSPI.h: Added. > > It is annoying to have this in a cocoa directory, because XPC has nothing to do with Cocoa. Maybe darwin? > Will move file to spi/darwin. > > Source/WTF/wtf/spi/cocoa/XPCSPI.h:87 > > +EXTERN_C CONST_ON_OR_AFTER_MAC_OS_X_VERSION_1090 struct _xpc_dictionary_s _xpc_error_connection_invalid; > > Where does EXTERN_C come from? EXTERN_C is defined in wtf/Compiler.h, <http://trac.webkit.org/browser/trunk/Source/WTF/wtf/Compiler.h?rev=174110#L160>. It was added in <http://trac.webkit.org/changeset/174110>. EXTERN_C is defined to be either 'extern "C"' or 'extern' depending on whether we are compiling a C++/Objective-C++ file or C/Objective-C file, respectively. > I'd prefer a plain extern "C" - it's no more verbose, but clearly less fragile. Notice that an extern "C"-block is only applicable in a C++/Objective-C++ source file since symbols declared in a C/Objective-C file have C-style linkage by default. So, we don't want 'extern "C"' to be in the source file. Otherwise, it will cause a compile error since "C" in 'extern "C"' is an unexpected string literal. It seems reasonable to support including the header XPCSPI.h from a C, Objective-C file (especially the latter). > > > Source/WTF/wtf/spi/cocoa/XPCSPI.h:99 > > +#if __BLOCKS__ > > We use "#ifdef __BLOCKS__" elsewhere in WebKit. Which is right? > #ifdef __BLOCKS__ is a better form. I noticed that we also have COMPILER_SUPPORTS(BLOCKS). Unless you feel strongly to use #ifdef __BLOCKS__, I'll change this line to read: #if COMPILER_SUPPORTS(BLOCKS) This will make querying for the Block language extension consistent with how we query for other compiler extensions/features. We should look to substitute COMPILER_SUPPORTS(BLOCKS) for __BLOCKS__ at other call sites. I suggest we do this in a separate patch. > > Source/WTF/wtf/spi/cocoa/XPCSPI.h:152 > > +#if !defined(xpc_retain) && OS_OBJECT_USE_OBJC_RETAIN_RELEASE > > +#define xpc_retain(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o retain]; }) > > +#endif > > + > > +#if !defined(xpc_release) && OS_OBJECT_USE_OBJC_RETAIN_RELEASE > > +#define xpc_release(object) ({ xpc_object_t _o = (object); _xpc_object_validate(_o); [_o release]; }) > > +#endif > > Does this compile in .cpp files? Notice these macro defines are conditional on compiling a Objective-C/C++ file with a compiler that support ARC (by definition of OS_OBJECT_USE_OBJC_RETAIN_RELEASE in os/object.h). So, a C++ file that includes XPCSPI.h will compile (since these macros are not applicable). Committed r174180: <http://trac.webkit.org/changeset/174180> Re-opened since this is blocked by bug 137320 Committed r174220: <http://trac.webkit.org/changeset/174220> |