Bug 16124 – Add printf format attribute to several functions to fix some GCC warnings

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

[Laszlo Gombos]

GCC has an extension to specify type check for format strings. See http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#Function-Attributes for details.

This syntax is already used in PlatformString.h and in DeprecatedString.h in WebCore.

This fix get rid of the following GCC warning
XMLTokenizer.cpp:881: warning: function might be possible candidate for ‘printf’ format attribute

[Laszlo Gombos]

Created attachment 17493 [details]
Fix attached.

[Maciej Stachowiak]

Comment on attachment 17493 [details]
Fix attached.

Substance of the change looks good. A few comments on style:

1) We usually use #if COMPILER(GCC) rather than #if __GNUC__

2) It might be a good idea to make a macro that's defined to either the right attribtue or nothing depending on the compiler, to avoid sprinking the code with compiler #ifdefs.

Please make those adjustments (feel free to also fix existing uses of the printf attribute).

r- for now, will gladly re-review with those fixes.

[Sam Weinig]

You can actually use the new WTF_ATTRIBUTE_PRINTF defined in wtf/Assertions.h for this.

[Laszlo Gombos]

Created attachment 17497 [details]
Revised patch

Maciej, could not agree more.
Sam, thanks for the tip.

Revised patch attached.

[Mark Rowe (bdash)]

Comment on attachment 17497 [details]
Revised patch

The attributes on warningHandler, fatalErrorHandler and friends should probably go on the functions themselves rather than forward-declaring them such as you have.

Why does handleError need the attribute?  It does not appear to be used with a format string.

[Laszlo Gombos]

Created attachment 17498 [details]
2nd revised patch

Mark, you're right about handleError(), I took it out from the patch. 

The format attribute can only be used in function declaration, that is why warningHandler() and friends needs to be forward-declared.

[Mark Rowe (bdash)]

(In reply to comment #6)
> Created an attachment (id=17498) [edit]
> 2nd revised patch
> 
> Mark, you're right about handleError(), I took it out from the patch. 
> 
> The format attribute can only be used in function declaration, that is why
> warningHandler() and friends needs to be forward-declared.

I don't believe that is the case.  See <http://trac.webkit.org/projects/webkit/browser/trunk/JavaScriptCore/wtf/Assertions.cpp?rev=27947#L50>, for instance.

[Sam Weinig]

Comment on attachment 17498 [details]
2nd revised patch

r- based on marks comments

[Laszlo Gombos]

Created attachment 17506 [details]
3rd revised patch

Mark, you're right, it compiles with GCC 4.1 - I checked. Revised patch attached.

[Sam Weinig]

Comment on attachment 17506 [details]
3rd revised patch

r=me

[Mark Rowe (bdash)]

Landed in r28028.  Thanks for being patient with the review process!

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.