Bug 117791 – Fixing some compiler warnings

• Component: New Bugs
• Status: RESOLVED FIXED

[Xabier Rodríguez Calvar]

Fixing some compiler warnings

[Xabier Rodríguez Calvar]

Created attachment 204989 [details]
Patch

Fixed some warnings.

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Build Bot]

Comment on attachment 204989 [details]
Patch

Attachment 204989 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/945263

[Xabier Rodríguez Calvar]

Created attachment 205032 [details]
Patch

Fixed windows problems, hopefully.

[Build Bot]

Comment on attachment 205032 [details]
Patch

Attachment 205032 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/904623

[Xabier Rodríguez Calvar]

Created attachment 205065 [details]
Patch

Fixed windows problems, again, hopefully.

[EFL EWS Bot]

Comment on attachment 205065 [details]
Patch

Attachment 205065 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/923781

[EFL EWS Bot]

Comment on attachment 205065 [details]
Patch

Attachment 205065 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/880478

[Early Warning System Bot]

Comment on attachment 205065 [details]
Patch

Attachment 205065 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/951479

[Xabier Rodríguez Calvar]

Created attachment 205067 [details]
Patch

Changelog issue.

[EFL EWS Bot]

Comment on attachment 205067 [details]
Patch

Attachment 205067 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/880480

[EFL EWS Bot]

Comment on attachment 205067 [details]
Patch

Attachment 205067 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/880483

[Early Warning System Bot]

Comment on attachment 205067 [details]
Patch

Attachment 205067 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/943476

[Early Warning System Bot]

Comment on attachment 205067 [details]
Patch

Attachment 205067 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/924685

[Xabier Rodríguez Calvar]

Created attachment 205070 [details]
Patch

Added WTF include dir to cmake in netscape plugin to build efl and qt properly.

[Early Warning System Bot]

Comment on attachment 205070 [details]
Patch

Attachment 205070 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/924692

[Early Warning System Bot]

Comment on attachment 205070 [details]
Patch

Attachment 205070 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/951485

[Xabier Rodríguez Calvar]

Created attachment 205084 [details]
Patch

Added WTF include dir to cmake in netscape plugin to build efl and qt properly.

[Martin Robinson]

Comment on attachment 205084 [details]
Patch

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

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:637
> +        offsetType = GAIL_AT_OFFSET; // to prevent a compiler warning

I'd prefer this to be defined when the variable is declared. Comments should be complete sentences with a capital letter and a period, but it's okay if you just omit these kind of comments completely.

> Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.h:31
> +#ifndef WTF_EXPORT_PRIVATE
> +#define WTF_EXPORT_PRIVATE
> +#endif

Is this usually defined in config.h?

> Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:47
> +#ifdef CRASH
> +#undef CRASH
> +#endif
> +

Maybe it would be better to simply pick a non-conflicting macro name?

> Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionNoCache_Bundle.cpp:223
> +        index = 0; // to avoid compiler warnings

I think it's better to set the value of 'index' when you declare it and omit the comment.

[Xabier Rodríguez Calvar]

(In reply to comment #19)
> (From update of attachment 205084 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205084&action=review
> 
> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:637
> > +        offsetType = GAIL_AT_OFFSET; // to prevent a compiler warning
> 
> I'd prefer this to be defined when the variable is declared. Comments should be complete sentences with a capital letter and a period, but it's okay if you just omit these kind of comments completely.

Roger that. I did it like this because initialization is supposed to happen always and the default only when something wrong happens, which would be rare. Anyway, compiler can optimize it in a different way and my assumption might be irrelevant.

> > Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.h:31
> > +#ifndef WTF_EXPORT_PRIVATE
> > +#define WTF_EXPORT_PRIVATE
> > +#endif
> 
> Is this usually defined in config.h?
> 
> > Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:47
> > +#ifdef CRASH
> > +#undef CRASH
> > +#endif
> > +
> 
> Maybe it would be better to simply pick a non-conflicting macro name?

These two cases come from including <wtf/Assertions.h>. When I add it, I get compilation errors because of those two cases, the first is not defined and the second is redefined. I could define the export in config.h and maybe change the CRASH macro at main.cpp so that it doesn't conflict with the one in the <wtf/Assertions.h> as you suggest and if I understood you correctly.

Other solution would be defining the WTF_ATTRIBUTE_PRINTF macro for those cases instead of just including the header that also causes the other problems.

> > Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionNoCache_Bundle.cpp:223
> > +        index = 0; // to avoid compiler warnings
> 
> I think it's better to set the value of 'index' when you declare it and omit the comment.

Same as in the first case.

[Xabier Rodríguez Calvar]

Created attachment 205382 [details]
Patch

Changed according Martin's comments.

[Build Bot]

Comment on attachment 205382 [details]
Patch

Attachment 205382 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/919110

[Early Warning System Bot]

Comment on attachment 205382 [details]
Patch

Attachment 205382 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/849378

[EFL EWS Bot]

Comment on attachment 205382 [details]
Patch

Attachment 205382 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/912669

[EFL EWS Bot]

Comment on attachment 205382 [details]
Patch

Attachment 205382 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/849377

[Build Bot]

Comment on attachment 205382 [details]
Patch

Attachment 205382 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/850837

[Early Warning System Bot]

Comment on attachment 205382 [details]
Patch

Attachment 205382 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/930294

[Build Bot]

Comment on attachment 205382 [details]
Patch

Attachment 205382 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/894248

[Xabier Rodríguez Calvar]

Created attachment 205480 [details]
Patch

Fixed Mac compilation and trying fixes for Qt and EFL

[EFL EWS Bot]

Comment on attachment 205480 [details]
Patch

Attachment 205480 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/919205

[EFL EWS Bot]

Comment on attachment 205480 [details]
Patch

Attachment 205480 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/946330

[Early Warning System Bot]

Comment on attachment 205480 [details]
Patch

Attachment 205480 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/919207

[Early Warning System Bot]

Comment on attachment 205480 [details]
Patch

Attachment 205480 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/930442

[Xabier Rodríguez Calvar]

Created attachment 205504 [details]
Patch

Including ExportMacros.h instead of the config.h mess.

[Martin Robinson]

Comment on attachment 205504 [details]
Patch

I think to fix a warning the changes to the plugin are too big and risky (for instance, linking against wtf). Let's just silence this warning on GTK+.

[Xabier Rodríguez Calvar]

Created attachment 205529 [details]
Patch

Silenced format warning for Gtk+ as required by Martin.

[Carlos Garcia Campos]

Comment on attachment 205529 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests needed.

You don't need to include this line when it's obvious, like in this case that you are fixing compile warnings.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:315
> +        0, // willLoadURLRequest;
> +        0, // willLoadDataRequest;

Those trailing ; are weird, but harmless anyway

[WebKit Commit Bot]

Comment on attachment 205529 [details]
Patch

Clearing flags on attachment: 205529

Committed r152095: <http://trac.webkit.org/changeset/152095>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.