Bug 16124 - Add printf format attribute to several functions to fix some GCC warnings
Summary: Add printf format attribute to several functions to fix some GCC warnings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Minor
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-11-24 18:38 PST by Laszlo Gombos
Modified: 2019-05-02 16:18 PDT (History)
4 users (show)

See Also:


Attachments
Fix attached. (2.65 KB, patch)
2007-11-24 18:39 PST, Laszlo Gombos
mjs: review-
Details | Formatted Diff | Diff
Revised patch (3.66 KB, patch)
2007-11-24 20:41 PST, Laszlo Gombos
mrowe: review-
Details | Formatted Diff | Diff
2nd revised patch (3.31 KB, patch)
2007-11-24 21:21 PST, Laszlo Gombos
sam: review-
Details | Formatted Diff | Diff
3rd revised patch (3.62 KB, patch)
2007-11-25 08:06 PST, Laszlo Gombos
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Gombos 2007-11-24 18:38:54 PST
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
Comment 1 Laszlo Gombos 2007-11-24 18:39:45 PST
Created attachment 17493 [details]
Fix attached.
Comment 2 Maciej Stachowiak 2007-11-24 19:08:31 PST
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.
Comment 3 Sam Weinig 2007-11-24 19:28:10 PST
You can actually use the new WTF_ATTRIBUTE_PRINTF defined in wtf/Assertions.h for this.
Comment 4 Laszlo Gombos 2007-11-24 20:41:38 PST
Created attachment 17497 [details]
Revised patch

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

Revised patch attached.
Comment 5 Mark Rowe (bdash) 2007-11-24 20:54:51 PST
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.
Comment 6 Laszlo Gombos 2007-11-24 21:21:44 PST
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.
Comment 7 Mark Rowe (bdash) 2007-11-24 23:04:58 PST
(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 8 Sam Weinig 2007-11-25 03:58:16 PST
Comment on attachment 17498 [details]
2nd revised patch

r- based on marks comments
Comment 9 Laszlo Gombos 2007-11-25 08:06:50 PST
Created attachment 17506 [details]
3rd revised patch

Mark, you're right, it compiles with GCC 4.1 - I checked. Revised patch attached.
Comment 10 Sam Weinig 2007-11-25 11:18:06 PST
Comment on attachment 17506 [details]
3rd revised patch

r=me
Comment 11 Mark Rowe (bdash) 2007-11-25 18:13:19 PST
Landed in r28028.  Thanks for being patient with the review process!
Comment 12 Lucas Forschler 2019-02-06 09:02:44 PST
Mass moving XML DOM bugs to the "DOM" Component.