These unused parameter warnings are making it difficult to spot important warnings. We've put off fixing them for years, so we should fix them now.
Created attachment 226738 [details] Patch
Attachment 226738 [details] did not pass style-queue: ERROR: Source/WebCore/plugins/gtk/gtk2xtbin.c:134: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/plugins/gtk/gtk2xtbin.c:135: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/plugins/gtk/gtk2xtbin.c:148: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/plugins/gtk/gtk2xtbin.c:168: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/plugins/gtk/gtk2xtbin.c:169: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/plugins/gtk/gtk2xtbin.c:170: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/plugins/gtk/gtk2xtbin.c:700: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/plugins/gtk/gtk2xtbin.c:837: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 8 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #2) > Attachment 226738 [details] did not pass style-queue: gtk2xtbin.c has never followed WebKit coding style, so these are false positives.
Comment on attachment 226738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226738&action=review Thanks! > Source/WebCore/platform/gtk/GtkVersioning.c:91 > + UNUSED_PARAM(srcX); > + Have you checked this isn't a bug? sounds weird we are not using srcX, but I haven't looked at the code. > Source/WebCore/platform/soup/SharedBufferSoup.cpp:44 > -void SharedBuffer::tryReplaceContentsWithPlatformBuffer(SharedBuffer* newContents) > +void SharedBuffer::tryReplaceContentsWithPlatformBuffer(SharedBuffer* /* newContents */) Maybe we can simply omit the parameter in this case. > Source/WebCore/plugins/gtk/PluginViewGtk.cpp:500 > -void PluginView::plugAddedCallback(GtkSocket* socket, PluginView* view) > +void PluginView::plugAddedCallback(GtkSocket*, PluginView* view) > { > ASSERT(socket); This is going to fail in debug builds. I think we can remove the assert, the callback will never be emitted with a NULL widget. > Source/WebCore/plugins/gtk/gtk2xtbin.c:135 > + UNUSED_PARAM(source_data); > + UNUSED_PARAM(timeout); We can't omit the names here because this file is C? > Source/WebCore/rendering/InlineTextBox.cpp:991 > +#if !ENABLE(CSS3_TEXT_DECORATION_SKIP_INK) > + UNUSED_PARAM(textPainter); > +#endif Why not adding an #else to the existing ifdef? because there's more than one in the function body?
(In reply to comment #4) > (From update of attachment 226738 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=226738&action=review > > Thanks! > > > Source/WebCore/platform/gtk/GtkVersioning.c:91 > > + UNUSED_PARAM(srcX); > > + > > Have you checked this isn't a bug? sounds weird we are not using srcX, but I haven't looked at the code. It was pretty suspicious to me too, but I haven't had a chance to look into it. I've uncovered at least two serious problems already fixing these issues! > > > Source/WebCore/platform/soup/SharedBufferSoup.cpp:44 > > -void SharedBuffer::tryReplaceContentsWithPlatformBuffer(SharedBuffer* newContents) > > +void SharedBuffer::tryReplaceContentsWithPlatformBuffer(SharedBuffer* /* newContents */) > > Maybe we can simply omit the parameter in this case. Sure. > > Source/WebCore/plugins/gtk/PluginViewGtk.cpp:500 > > -void PluginView::plugAddedCallback(GtkSocket* socket, PluginView* view) > > +void PluginView::plugAddedCallback(GtkSocket*, PluginView* view) > > { > > ASSERT(socket); > > This is going to fail in debug builds. I think we can remove the assert, the callback will never be emitted with a NULL widget. Nice catch! We should probably replace it with ASSERT_UNUSED. > > Source/WebCore/plugins/gtk/gtk2xtbin.c:135 > > + UNUSED_PARAM(source_data); > > + UNUSED_PARAM(timeout); > > We can't omit the names here because this file is C? That's right. > > Source/WebCore/rendering/InlineTextBox.cpp:991 > > +#if !ENABLE(CSS3_TEXT_DECORATION_SKIP_INK) > > + UNUSED_PARAM(textPainter); > > +#endif > > Why not adding an #else to the existing ifdef? because there's more than one in the function body? Yes. I think it makes sense to have the UNUSED_PARAM at the start of the function too. Having it in a predictable location will likely be better for visibility.
Committed r165688: <http://trac.webkit.org/changeset/165688>