Bug 146007 – Remove build warnings in Tools/DumpRenderTree/TestNetscapePlugIn

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Tanay]

../../Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:68:108: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
     messageLength += vsnprintf(message + messageLength, messageBufferSize - 1 - messageLength, format, args);
                                                                                                            ^
../../Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:68:108: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]

[Tanay]

Created attachment 254937 [details]
Patch

[Darin Adler]

Comment on attachment 254937 [details]
Patch

I am pretty sure you could use attribute rather than __attribute__. In macros it makes sense to use the form less likely to conflict, but in code like this it doesn’t seem necessary.

[Darin Adler]

Comment on attachment 254937 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254937&action=review

For best compatibility across platforms would be much better to use WTF_ATTRIBUTE_PRINTF. Please do that.

> Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:63
> +void __attribute__((format(printf, 2, 0))) pluginLogWithArguments(NPP instance, const char* format, va_list args)

Oops, no this is wrong. Needs to use WTF_ATTRIBUTE_PRINTF

[Gyuyoung Kim]

Comment on attachment 254937 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254937&action=review

> Tools/ChangeLog:3
> +        [EFL] Remove build warnings in Tools/DumpRenderTree/TestNetscapePlugIn

I think this patch isn't only for EFL port. Please remove [EFL] prefix or add other ports too.

[Tanay]

I am getting compilation errors if this macro is used. 

It seems like the 'WTF_ATTRIBUTE_PRINTF' macro is not available in this directory.  

This is conditionally compiled for ENABLE_X11_TARGET flag. 
      if (ENABLE_X11_TARGET)
        add_subdirectory(DumpRenderTree/TestNetscapePlugIn)

Please advice.

[Darin Adler]

(In reply to comment #5)
> I am getting compilation errors if this macro is used. 
> 
> It seems like the 'WTF_ATTRIBUTE_PRINTF' macro is not available in this
> directory.  

I don’t think it’s the directory, but rather the includes. You’ll have to include an appropriate WTF header file to use WTF_ATTRIBUTE_PRINTF. It doesn’t compile because it’s not including the header file. I think it’s <wtf/Assertions.h>.

> This is conditionally compiled for ENABLE_X11_TARGET flag. 
>       if (ENABLE_X11_TARGET)
>         add_subdirectory(DumpRenderTree/TestNetscapePlugIn)

That’s in the make file used for the EFL port. This same plug-in is built, just built with other build systems, for other ports that don’t necessarily share that make file.

[Tanay]

After including the <wtf/Assertions.h> header and I get the following error

../../Source/WTF/wtf/Assertions.h:130:1: error: ‘WTF_EXPORT_PRIVATE’ does not name a type

[Tanay]

Are there some additional dependencies that I am missing?
I have tried linking with the WTF library but it does not resolve.

[Csaba Osztrogonác]

(In reply to comment #8)
> Are there some additional dependencies that I am missing?
> I have tried linking with the WTF library but it does not resolve.

Assertions.h needs WTF_EXPORT_PRIVATE, which is defined in wtf/ExportMacros.h.
And wtf/ExportMacros.h needs OS/USE/COMPILER macros which are defined in Platform.h.

Adding these includes in exactly this order + adding WTF to include path
in cmake build system solves this issue ... But it is very ugly and
style checker will warn because of unordered includes ...

#include <wtf/Platform.h>
#include <wtf/ExportMacros.h>
#include <wtf/Assertions.h>

[Tanay]

Created attachment 255187 [details]
Patch

[Tanay]

Thanks for the clarifications. Uploaded a patch with the changes.

[Darin Adler]

I’m surprised that Assertions.h does not itself include those other headers. I think maybe it’s because we expect those other headers to be included universally as part of config.h or precompiled headers.

[WebKit Commit Bot]

Comment on attachment 255187 [details]
Patch

Rejecting attachment 255187 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 255187, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
zz 3.
patching file Tools/DumpRenderTree/TestNetscapePlugIn/CMakeLists.txt
Hunk #1 FAILED at 33.
1 out of 1 hunk FAILED -- saving rejects to file Tools/DumpRenderTree/TestNetscapePlugIn/CMakeLists.txt.rej
patching file Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp
patching file Tools/DumpRenderTree/TestNetscapePlugIn/PluginTest.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/4736415837454336

[Tanay]

Created attachment 255342 [details]
Patch

[Tanay]

Re-based and uploaded the patch.

[WebKit Commit Bot]

Comment on attachment 255342 [details]
Patch

Clearing flags on attachment: 255342

Committed r185871: <http://trac.webkit.org/changeset/185871>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.