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
Created attachment 17493 [details] Fix attached.
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.
You can actually use the new WTF_ATTRIBUTE_PRINTF defined in wtf/Assertions.h for this.
Created attachment 17497 [details] Revised patch Maciej, could not agree more. Sam, thanks for the tip. Revised patch attached.
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.
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.
(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.
Comment on attachment 17498 [details] 2nd revised patch r- based on marks comments
Created attachment 17506 [details] 3rd revised patch Mark, you're right, it compiles with GCC 4.1 - I checked. Revised patch attached.
Comment on attachment 17506 [details] 3rd revised patch r=me
Landed in r28028. Thanks for being patient with the review process!
Mass moving XML DOM bugs to the "DOM" Component.