Bug 25087 – Test for ENABLE_FOO macros consistently in IDL files

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[David Kilzer (:ddkilzer)]

* 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.

[David Kilzer (:ddkilzer)]

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?

[Eric Seidel (no email)]

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?

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

(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.)

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> Change tests for ENABLE macros to check for both existance and value:

Typo: existence

[Timothy Hatcher]

Comment on attachment 30202 [details]
Patch v2

Thanks for fixing this!

[David Kilzer (:ddkilzer)]

$ 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

[Alexey Proskuryakov]

It would be nice to have an explanation for posterity about why

#if defined(ENABLE_FOO) && ENABLE_FOO

is better than

#if ENABLE_FOO

[David Kilzer (:ddkilzer)]

(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.)

[David Kilzer (:ddkilzer)]

(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.