WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
96742
Fix unused parameter compile warnings in WebKit/WebKit2
https://bugs.webkit.org/show_bug.cgi?id=96742
Summary
Fix unused parameter compile warnings in WebKit/WebKit2
Jinwoo Song
Reported
2012-09-14 02:52:42 PDT
Fix remained unused parameter warnings during EFL build. Patch is coming.
Attachments
patch
(101.46 KB, patch)
2012-09-14 22:56 PDT
,
Jinwoo Song
no flags
Details
Formatted Diff
Diff
patch
(101.49 KB, patch)
2012-09-15 00:36 PDT
,
Jinwoo Song
no flags
Details
Formatted Diff
Diff
patch.
(88.96 KB, patch)
2012-09-17 04:48 PDT
,
Jinwoo Song
no flags
Details
Formatted Diff
Diff
patch.
(88.00 KB, patch)
2012-09-17 05:33 PDT
,
Jinwoo Song
no flags
Details
Formatted Diff
Diff
patch
(101.46 KB, patch)
2012-09-17 21:44 PDT
,
Jinwoo Song
no flags
Details
Formatted Diff
Diff
patch
(103.83 KB, patch)
2012-09-18 05:46 PDT
,
Jinwoo Song
no flags
Details
Formatted Diff
Diff
patch applying Gyuyoung's comments.
(101.97 KB, patch)
2012-09-18 20:21 PDT
,
Jinwoo Song
gyuyoung.kim
: review+
Details
Formatted Diff
Diff
patch for landing
(101.84 KB, patch)
2012-09-19 18:45 PDT
,
Jinwoo Song
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
patch.
(101.84 KB, patch)
2012-09-19 22:08 PDT
,
Jinwoo Song
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Jinwoo Song
Comment 1
2012-09-14 22:56:49 PDT
Created
attachment 164274
[details]
patch
Jinwoo Song
Comment 2
2012-09-15 00:36:25 PDT
Created
attachment 164278
[details]
patch
Chris Dumez
Comment 3
2012-09-15 01:05:52 PDT
I think it would be better to use the UNUSED_PARAM() macro instead of removing the argument names. We loose useful information with this patch.
Gyuyoung Kim
Comment 4
2012-09-15 01:45:23 PDT
(In reply to
comment #3
)
> I think it would be better to use the UNUSED_PARAM() macro instead of removing the argument names. We loose useful information with this patch.
I also agree to use UNUSED_PARAM(), but, I think we don't even need to use it for unimplemented functions.
Jinwoo Song
Comment 5
2012-09-17 04:48:07 PDT
Created
attachment 164370
[details]
patch. I used the UNUSED_PARAM() macro for not used arguments and removed the arguments which are meaningless as the parameter type name contains the parameter name.
WebKit Review Bot
Comment 6
2012-09-17 04:51:56 PDT
Attachment 164370
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1 Source/WebKit2/WebProcess/WebPage/DrawingArea.h:113: Missing space inside { }. [whitespace/braces] [5] Total errors found: 1 in 64 files If any of these errors are false positives, please file a bug against check-webkit-style.
Gyuyoung Kim
Comment 7
2012-09-17 04:52:51 PDT
Comment on
attachment 164370
[details]
patch. View in context:
https://bugs.webkit.org/attachment.cgi?id=164370&action=review
> Source/WebKit/efl/ChangeLog:3 > + [EFL] Fix unused parameter compile warnings in WebKit/WebKit2
You have to remove [EFL] prefix because this patch touches common files as well as efl files.
Jinwoo Song
Comment 8
2012-09-17 05:33:57 PDT
Created
attachment 164374
[details]
patch. Changed the title and fixed the style error.
Gyuyoung Kim
Comment 9
2012-09-17 07:22:43 PDT
Comment on
attachment 164374
[details]
patch. Looks fine on EFL port side.
Gyuyoung Kim
Comment 10
2012-09-17 07:43:51 PDT
Comment on
attachment 164374
[details]
patch. View in context:
https://bugs.webkit.org/attachment.cgi?id=164374&action=review
> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:159 > + UNUSED_PARAM(frameLoadRequest);
I missed something stuff. I think it is better to only use UNUSED_PARAM in functions inside which has #if ENABLE(). Because, excessive UNUSED_PARAM usage makes noise in code, IMO.
r128570
also did same.
> Source/WebKit/efl/WebCoreSupport/FullscreenVideoControllerEfl.cpp:63 > + UNUSED_PARAM(autoHide);
Just remove parameter in unimplemented function.
Jinwoo Song
Comment 11
2012-09-17 07:59:50 PDT
(In reply to
comment #10
)
> (From update of
attachment 164374
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=164374&action=review
> > > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:159 > > + UNUSED_PARAM(frameLoadRequest); > > I missed something stuff. I think it is better to only use UNUSED_PARAM in functions inside which has #if ENABLE(). Because, excessive UNUSED_PARAM usage makes noise in code, IMO.
r128570
also did same.
Actually, my second patch is exactly the one you explain. Do you want to revert the patch to second one?
> > > Source/WebKit/efl/WebCoreSupport/FullscreenVideoControllerEfl.cpp:63 > > + UNUSED_PARAM(autoHide); > > Just remove parameter in unimplemented function.
Gyuyoung Kim
Comment 12
2012-09-17 08:19:22 PDT
(In reply to
comment #11
)
> Actually, my second patch is exactly the one you explain. Do you want to revert the patch to second one?
Yes, I prefer to use second patch.
Jinwoo Song
Comment 13
2012-09-17 21:44:06 PDT
Created
attachment 164489
[details]
patch
Jinwoo Song
Comment 14
2012-09-17 21:48:56 PDT
(In reply to
comment #12
)
> (In reply to
comment #11
) > > Actually, my second patch is exactly the one you explain. Do you want to revert the patch to second one? > > Yes, I prefer to use second patch.
Done. :)
Mikhail Pozdnyakov
Comment 15
2012-09-17 23:45:58 PDT
Comment on
attachment 164489
[details]
patch View in context:
https://bugs.webkit.org/attachment.cgi?id=164489&action=review
> Source/WebKit2/WebProcess/WebPage/DrawingArea.h:73 > + virtual bool forceRepaintAsync(uint64_t) { return false; }
Is it now clear that forceRepaintAsync accepts callback?
> Source/WebKit2/WebProcess/WebPage/DrawingArea.h:113 > + virtual void updateBackingStoreState(uint64_t, bool, float, const WebCore::IntSize&, const WebCore::IntSize&) { }
The method became very mysterious :) Now it's pretty hard to guess what its arguments mean.
> Source/WebKit2/WebProcess/WebPage/LayerTreeHost.h:76 > + virtual bool forceRepaintAsync(uint64_t) { return false; }
Same here.. I think we should not sacrifice with code readability just in order to avoid compiler warnings
Mikhail Pozdnyakov
Comment 16
2012-09-17 23:52:37 PDT
***
Bug 96901
has been marked as a duplicate of this bug. ***
Gyuyoung Kim
Comment 17
2012-09-17 23:59:16 PDT
(In reply to
comment #15
)
> (From update of
attachment 164489
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=164489&action=review
> > > Source/WebKit2/WebProcess/WebPage/DrawingArea.h:73 > > + virtual bool forceRepaintAsync(uint64_t) { return false; } > > Is it now clear that forceRepaintAsync accepts callback? > > > Source/WebKit2/WebProcess/WebPage/DrawingArea.h:113 > > + virtual void updateBackingStoreState(uint64_t, bool, float, const WebCore::IntSize&, const WebCore::IntSize&) { } > > The method became very mysterious :) Now it's pretty hard to guess what its arguments mean. > > > Source/WebKit2/WebProcess/WebPage/LayerTreeHost.h:76 > > + virtual bool forceRepaintAsync(uint64_t) { return false; } > > Same here.. I think we should not sacrifice with code readability just in order to avoid compiler warnings
The
r128570
removed all unused parameters. However, I also worry about disturbing code readability again. Excessive UNUSED_PARAM disturbs code readability. On the other hands, to remove all parameters disturbs it as well. Why don't we use UNUSED_PARAM only for both #if ENABLE inside and primitive parameter ?
Mikhail Pozdnyakov
Comment 18
2012-09-18 00:30:42 PDT
> The
r128570
removed all unused parameters. However, I also worry about disturbing code readability again. Excessive UNUSED_PARAM disturbs code readability. On the other hands, to remove all parameters disturbs it as well. Why don't we use UNUSED_PARAM only for both #if ENABLE inside and primitive parameter ?
Think, we could keep variable name in /* */ (i.e. void doSomething(uint64_t /*trickyCallback*/ ); ) and also please take a look at
bug96985
.
Gyuyoung Kim
Comment 19
2012-09-18 00:34:54 PDT
(In reply to
comment #18
)
> > The
r128570
removed all unused parameters. However, I also worry about disturbing code readability again. Excessive UNUSED_PARAM disturbs code readability. On the other hands, to remove all parameters disturbs it as well. Why don't we use UNUSED_PARAM only for both #if ENABLE inside and primitive parameter ? > > Think, we could keep variable name in /* */ (i.e. void doSomething(uint64_t /*trickyCallback*/ ); ) and also please take a look at
bug96985
.
Good suggestion. BTW, It looks /* */ is better for me.
Jinwoo Song
Comment 20
2012-09-18 00:44:53 PDT
(In reply to
comment #19
)
> (In reply to
comment #18
) > > > The
r128570
removed all unused parameters. However, I also worry about disturbing code readability again. Excessive UNUSED_PARAM disturbs code readability. On the other hands, to remove all parameters disturbs it as well. Why don't we use UNUSED_PARAM only for both #if ENABLE inside and primitive parameter ? > > > > Think, we could keep variable name in /* */ (i.e. void doSomething(uint64_t /*trickyCallback*/ ); ) and also please take a look at
bug96985
. > > Good suggestion. BTW, It looks /* */ is better for me.
Thanks for good suggestion, Mikhail and Gyuyoung. I'll apply the /* */ for the primitive parameters.
Jinwoo Song
Comment 21
2012-09-18 05:46:38 PDT
Created
attachment 164539
[details]
patch Keep the primitive variable names in /* */.
Gyuyoung Kim
Comment 22
2012-09-18 19:08:46 PDT
Comment on
attachment 164539
[details]
patch View in context:
https://bugs.webkit.org/attachment.cgi?id=164539&action=review
> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:396 > +void ChromeClientEfl::reachedMaxAppCacheSize(int64_t)
/* spaceNeeded */ ?
> Source/WebKit/efl/ewk/ewk_frame.cpp:1828 > +WTF::PassRefPtr<WebCore::Widget> ewk_frame_plugin_create(Evas_Object* ewkFrame, const WebCore::IntSize& pluginSize, WebCore::HTMLPlugInElement* element, const WebCore::KURL& url, const WTF::Vector<WTF::String>& paramName, const WTF::Vector<WTF::String>& paramValues, const WTF::String& mimeType, bool loadManually)
I think paramNames is proper for Vector.
> Source/WebKit/efl/ewk/ewk_view.cpp:620 > +static void _ewk_view_smart_add_console_message(Ewk_View_Smart_Data*, const char* message, unsigned lineNumber, const char* sourceID)
You should not change *unsigned int* to *unsigned* in this patch.
> Source/WebKit/efl/ewk/ewk_view.cpp:1040 > +static void _ewk_view_smart_move(Evas_Object* ewkView, Evas_Coord, Evas_Coord)
Need to use /* */ for x, y.
> Source/WebKit/efl/ewk/ewk_view.cpp:4481 > +bool ewk_view_accelerated_compositing_object_create(Evas_Object* /*ewkView*/, Evas_Native_Surface* /*nativeSurface*/, const WebCore::IntRect& /*rect*/)
Unneeded to wrap /* */ to ewkView and nativeSurface.
> Source/WebKit/efl/ewk/ewk_view.cpp:4487 > +WebCore::GraphicsContext3D* ewk_view_accelerated_compositing_context_get(Evas_Object* /*ewkView*/)
Unneeded /* */ because ewkView is clear in this file.
> Source/WebKit/efl/ewk/ewk_view_tiled.cpp:49 > +static void* _ewk_view_tiled_updates_process_pre(void* data, Evas_Object* /*ewkView*/)
ditto.
> Source/WebKit/efl/ewk/ewk_view_tiled.cpp:66 > +_ewk_view_tiled_contents_size_changed_cb(void* data, Evas_Object* /*ewkView*/, void* eventInfo)
ditto.
> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:292 > +static void _ewk_view_on_focus_out(void* data, Evas*, Evas_Object* /*ewkView*/, void* /*eventInfo*/)
ditto.
> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:300 > +static void _ewk_view_on_mouse_wheel(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo)
ditto.
> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:309 > +static void _ewk_view_on_mouse_down(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo)
ditto.
> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:318 > +static void _ewk_view_on_mouse_up(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo)
ditto.
> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:327 > +static void _ewk_view_on_mouse_move(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo)
ditto.
> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:336 > +static void _ewk_view_on_key_down(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo)
ditto.
> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:345 > +static void _ewk_view_on_key_up(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo)
ditto.
> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:455 > +static void _ewk_view_smart_move(Evas_Object* ewkView, Evas_Coord, Evas_Coord)
ditto.
> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:266 > +void WebChromeClient::addMessageToConsole(MessageSource, MessageType, MessageLevel, const String& message, unsigned lineNumber, const String& /*sourceID*/)
ditto.
Jinwoo Song
Comment 23
2012-09-18 20:19:57 PDT
Comment on
attachment 164539
[details]
patch View in context:
https://bugs.webkit.org/attachment.cgi?id=164539&action=review
>> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:396 >> +void ChromeClientEfl::reachedMaxAppCacheSize(int64_t) > > /* spaceNeeded */ ?
fixed.
>> Source/WebKit/efl/ewk/ewk_frame.cpp:1828 >> +WTF::PassRefPtr<WebCore::Widget> ewk_frame_plugin_create(Evas_Object* ewkFrame, const WebCore::IntSize& pluginSize, WebCore::HTMLPlugInElement* element, const WebCore::KURL& url, const WTF::Vector<WTF::String>& paramName, const WTF::Vector<WTF::String>& paramValues, const WTF::String& mimeType, bool loadManually) > > I think paramNames is proper for Vector.
fixed the typo. Nice eye to catch the 's'.
>> Source/WebKit/efl/ewk/ewk_view.cpp:620 >> +static void _ewk_view_smart_add_console_message(Ewk_View_Smart_Data*, const char* message, unsigned lineNumber, const char* sourceID) > > You should not change *unsigned int* to *unsigned* in this patch.
okay. I'll not change it in this patch.
>> Source/WebKit/efl/ewk/ewk_view.cpp:1040 >> +static void _ewk_view_smart_move(Evas_Object* ewkView, Evas_Coord, Evas_Coord) > > Need to use /* */ for x, y.
fixed.
>> Source/WebKit/efl/ewk/ewk_view.cpp:4481 >> +bool ewk_view_accelerated_compositing_object_create(Evas_Object* /*ewkView*/, Evas_Native_Surface* /*nativeSurface*/, const WebCore::IntRect& /*rect*/) > > Unneeded to wrap /* */ to ewkView and nativeSurface.
fixed.
>> Source/WebKit/efl/ewk/ewk_view.cpp:4487 >> +WebCore::GraphicsContext3D* ewk_view_accelerated_compositing_context_get(Evas_Object* /*ewkView*/) > > Unneeded /* */ because ewkView is clear in this file.
fixed.
>> Source/WebKit/efl/ewk/ewk_view_tiled.cpp:49 >> +static void* _ewk_view_tiled_updates_process_pre(void* data, Evas_Object* /*ewkView*/) > > ditto.
fixed.
>> Source/WebKit/efl/ewk/ewk_view_tiled.cpp:66 >> +_ewk_view_tiled_contents_size_changed_cb(void* data, Evas_Object* /*ewkView*/, void* eventInfo) > > ditto.
fixed.
>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:292 >> +static void _ewk_view_on_focus_out(void* data, Evas*, Evas_Object* /*ewkView*/, void* /*eventInfo*/) > > ditto.
fixed.
>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:300 >> +static void _ewk_view_on_mouse_wheel(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo) > > ditto.
fixed.
>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:309 >> +static void _ewk_view_on_mouse_down(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo) > > ditto.
fixed.
>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:327 >> +static void _ewk_view_on_mouse_move(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo) > > ditto.
fixed.
>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:336 >> +static void _ewk_view_on_key_down(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo) > > ditto.
fixed.
>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:345 >> +static void _ewk_view_on_key_up(void* data, Evas*, Evas_Object* /*ewkView*/, void* eventInfo) > > ditto.
fixed.
>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:455 >> +static void _ewk_view_smart_move(Evas_Object* ewkView, Evas_Coord, Evas_Coord) > > ditto.
fixed.
>> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:266 >> +void WebChromeClient::addMessageToConsole(MessageSource, MessageType, MessageLevel, const String& message, unsigned lineNumber, const String& /*sourceID*/) > > ditto.
fixed.
Jinwoo Song
Comment 24
2012-09-18 20:21:49 PDT
Created
attachment 164649
[details]
patch applying Gyuyoung's comments.
WebKit Review Bot
Comment 25
2012-09-18 20:24:33 PDT
Attachment 164649
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1 Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:266: Omit int when using unsigned [runtime/unsigned] [1] Source/WebKit/efl/ewk/ewk_view.cpp:620: Omit int when using unsigned [runtime/unsigned] [1] Total errors found: 2 in 68 files If any of these errors are false positives, please file a bug against check-webkit-style.
Jinwoo Song
Comment 26
2012-09-18 20:26:04 PDT
(In reply to
comment #25
)
>
Attachment 164649
[details]
did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1 > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:266: Omit int when using unsigned [runtime/unsigned] [1] > Source/WebKit/efl/ewk/ewk_view.cpp:620: Omit int when using unsigned [runtime/unsigned] [1] > Total errors found: 2 in 68 files > > > If any of these errors are false positives, please file a bug against check-webkit-style.
According to Gyuyoung's comment, I did not fixed the 'unsigned int' to 'unsigned' in this patch.
Jinwoo Song
Comment 27
2012-09-19 18:45:02 PDT
Created
attachment 164819
[details]
patch for landing This patch only fixes the warnings related to unused variables.
WebKit Review Bot
Comment 28
2012-09-19 18:46:32 PDT
Attachment 164819
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1 Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:266: Omit int when using unsigned [runtime/unsigned] [1] Source/WebKit/efl/ewk/ewk_view.cpp:620: Omit int when using unsigned [runtime/unsigned] [1] Total errors found: 2 in 68 files If any of these errors are false positives, please file a bug against check-webkit-style.
Ryuan Choi
Comment 29
2012-09-19 21:52:56 PDT
Comment on
attachment 164819
[details]
patch for landing Tried.
WebKit Review Bot
Comment 30
2012-09-19 21:54:51 PDT
Comment on
attachment 164819
[details]
patch for landing Rejecting
attachment 164819
[details]
from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/efl/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output:
http://queues.webkit.org/results/13935055
Jinwoo Song
Comment 31
2012-09-19 22:08:37 PDT
Created
attachment 164837
[details]
patch. Add the reviewer's name in the ChangeLog.
WebKit Review Bot
Comment 32
2012-09-19 22:10:10 PDT
Attachment 164837
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1 Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:266: Omit int when using unsigned [runtime/unsigned] [1] Source/WebKit/efl/ewk/ewk_view.cpp:620: Omit int when using unsigned [runtime/unsigned] [1] Total errors found: 2 in 68 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 33
2012-09-19 23:52:36 PDT
Comment on
attachment 164837
[details]
patch. Clearing flags on attachment: 164837 Committed
r129096
: <
http://trac.webkit.org/changeset/129096
>
WebKit Review Bot
Comment 34
2012-09-19 23:52:41 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug