* SUMMARY Currently IDL files have a mix of macro testing styles: #if ENABLE_FOO #if defined(ENABLE_FOO) There should be only one style used and it should be used consistently.
Created attachment 29322 [details] Patch v1 Proposed fix. All the ports appear to be using the generate-bindings.pl script and only defining macros if they intend the feature to be enabled, so I propose changing: #if ENABLE_FOO to: #if defined(ENABLE_FOO) rather than: #if defined(ENABLE_FOO) && ENABLE_FOO Since there is currently a mix of the first two (and there are no apparent build issues), I think this is safe to do. Comments?
Comment on attachment 29322 [details] Patch v1 Seems something like ENABLE(SVG) would read better. I guess this is fine though. WebKit can haz real idl parser/preprocessor plz?
Comment on attachment 29322 [details] Patch v1 Actually, an internal review asked them to be changed to: #if defined(ENABLE_FOO) && ENABLE_FOO Clearing review flag to redo the patch.
(In reply to comment #2) > WebKit can haz real idl parser/preprocessor plz? Sounds like a discussion topic for webkit-dev. (Also, I'm not sure what makes a real vs. non-real IDL parser.)
Created attachment 30202 [details] Patch v2 Change tests for ENABLE macros to check for both existance and value: - Negative: #if !defined(ENABLE_FOO) || !ENABLE_FOO - Positive: #if defined(ENABLE_FOO) && ENABLE_FOO
(In reply to comment #5) > Change tests for ENABLE macros to check for both existance and value: Typo: existence
Comment on attachment 30202 [details] Patch v2 Thanks for fixing this!
$ git svn dcommit Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/css/CSSCharsetRule.idl M WebCore/css/CSSPrimitiveValue.idl M WebCore/css/RGBColor.idl M WebCore/dom/Attr.idl M WebCore/dom/DOMCoreException.idl M WebCore/dom/DOMImplementation.idl M WebCore/dom/Document.idl M WebCore/dom/Element.idl M WebCore/dom/Event.idl M WebCore/dom/EventException.idl M WebCore/dom/KeyboardEvent.idl M WebCore/dom/MessagePort.idl M WebCore/dom/MouseEvent.idl M WebCore/dom/Node.idl M WebCore/dom/ProcessingInstruction.idl M WebCore/dom/Range.idl M WebCore/dom/RangeException.idl M WebCore/dom/WheelEvent.idl M WebCore/html/CanvasPixelArray.idl M WebCore/html/HTMLAnchorElement.idl M WebCore/html/HTMLAppletElement.idl M WebCore/html/HTMLAreaElement.idl M WebCore/html/HTMLBaseFontElement.idl M WebCore/html/HTMLCanvasElement.idl M WebCore/html/HTMLDocument.idl M WebCore/html/HTMLElement.idl M WebCore/html/HTMLEmbedElement.idl M WebCore/html/HTMLFrameElement.idl M WebCore/html/HTMLIFrameElement.idl M WebCore/html/HTMLImageElement.idl M WebCore/html/HTMLInputElement.idl M WebCore/html/HTMLLinkElement.idl M WebCore/html/HTMLObjectElement.idl M WebCore/html/HTMLOptionElement.idl M WebCore/html/HTMLOptionsCollection.idl M WebCore/html/HTMLSelectElement.idl M WebCore/html/HTMLStyleElement.idl M WebCore/html/ImageData.idl M WebCore/inspector/InspectorController.idl M WebCore/loader/appcache/DOMApplicationCache.idl M WebCore/page/Console.idl M WebCore/page/Coordinates.idl M WebCore/page/DOMSelection.idl M WebCore/page/DOMWindow.idl M WebCore/page/Geoposition.idl M WebCore/page/History.idl M WebCore/page/Location.idl M WebCore/page/Navigator.idl M WebCore/svg/SVGElementInstance.idl M WebCore/svg/SVGException.idl M WebCore/workers/WorkerContext.idl M WebCore/xml/XMLHttpRequestException.idl M WebCore/xml/XPathException.idl Committed r43528 http://trac.webkit.org/changeset/43528
It would be nice to have an explanation for posterity about why #if defined(ENABLE_FOO) && ENABLE_FOO is better than #if ENABLE_FOO
(In reply to comment #9) > It would be nice to have an explanation for posterity about why > > #if defined(ENABLE_FOO) && ENABLE_FOO > > is better than > > #if ENABLE_FOO My understanding is that it's safer, so that this is insufficient: #define ENABLE_FOO Thus you must specifically set ENABLE_FOO to a boolean true value to enable the feature: #define ENABLE_FOO 1 Also, the above double-check is what the ENABLE() macro does in JavaScriptCore/wtf/Platform.h, so doing the same thing above matches behavior in the source code. If we got fancy, we could #include <wtf/Platform.h> when preprocessing IDL files so that we could just say this instead: #if ENABLE(FOO) Perhaps that's worth a follow-up bug? (The tricky part about doing this is getting the compiler flags to reasonably match when including the Platform.h file so that you get the same behavior when preprocessing IDL files as you do when compiling source files. Maybe it's enough to duplicate just the ENABLE() macro definition for this purpose.)
(In reply to comment #10) > Perhaps that's worth a follow-up bug? (The tricky part about doing this is > getting the compiler flags to reasonably match when including the Platform.h > file so that you get the same behavior when preprocessing IDL files as you do > when compiling source files. Maybe it's enough to duplicate just the ENABLE() > macro definition for this purpose.) I filed Bug 25727.