Fix build warnings : -Wunused-parameter, -Wparentheses, -Wuninitialized.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Byungwoo Lee]

There is build warning.
 - ewk_view_loader_client.cpp : -Wunused-parameter
 - ewk_frame.cpp : Wparentheses

[Byungwoo Lee]

Created attachment 165075 [details]
Patch

[Kangil Han]

Comment on attachment 165075 [details]
Patch

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

Patch itself is good. Thanks!

> Source/WebKit/efl/ChangeLog:8
> +        Fix build warning : -Wparentheses.

This can be more specific.

> Source/WebKit2/ChangeLog:8
> +        Fix build warning : -Wunused-parameter

This can be more specific.

[Byungwoo Lee]

(In reply to comment #2)
> (From update of attachment 165075 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165075&action=review
> 
> Patch itself is good. Thanks!
> 
> > Source/WebKit/efl/ChangeLog:8
> > +        Fix build warning : -Wparentheses.
> 
> This can be more specific.
> 
> > Source/WebKit2/ChangeLog:8
> > +        Fix build warning : -Wunused-parameter
> 
> This can be more specific.

Ok~ I'll add more description.
Thanks :)

[Byungwoo Lee]

Created attachment 165109 [details]
Patch

[Kangil Han]

LGTM, thanks!

[Byungwoo Lee]

(In reply to comment #5)
> LGTM, thanks!

Thanks for your review.
I found some more build warnings at the latest source.
So I added the fixes for those with this.

[Byungwoo Lee]

Created attachment 165274 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 165274 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        No new tests.

Need to mention why this patch doesn't need to have test case.

[Benjamin Poulain]

Comment on attachment 165274 [details]
Patch

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

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:101
>  static void pthreadSignalHandlerSuspendResume(int signo)
>  {
> +    UNUSED_PARAM(signo);

You should remove the parameter name in this case.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:306
>  static SlowPathReturnType entryOSR(ExecState* exec, Instruction* pc, CodeBlock* codeBlock, const char *name, EntryKind kind)
>  {
> +    UNUSED_PARAM(pc);

Can't you remove the parameter entirely?
If not, remove the name.

> Source/JavaScriptCore/runtime/DatePrototype.cpp:201
>  static JSCell* formatLocaleDate(ExecState* exec, DateInstance* dateObject, double timeInMilliseconds, LocaleDateTimeFormat format)
>  {
> +    UNUSED_PARAM(dateObject);
>      UDateFormatStyle timeStyle = (format != LocaleDate ? UDAT_LONG : UDAT_NONE);

Remove the parameter name.

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:273
> +    UNUSED_PARAM(mimeType);
>      ASSERT(mimeType == "image/png"); // Only PNG output is supported for now.

This should be ASSERT_UNUSED

> Source/WebCore/platform/image-decoders/ImageDecoder.h:294
> +            UNUSED_PARAM(profileLength);
>              ASSERT(profileLength >= iccColorProfileHeaderLength);

Ditto.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:302
> +            UNUSED_PARAM(profileLength);
>              ASSERT(profileLength >= iccColorProfileHeaderLength);

Ditto.

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:661
> +    UNUSED_PARAM(iconType);

Ditto.

> Source/WebKit2/Platform/CoreIPC/Connection.cpp:430
> +    UNUSED_PARAM(syncRequestID);

Remove syncRequestID.

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:64
> +    UNUSED_PARAM(adoptTag);
>      ASSERT(adoptTag == AdoptWK); // Guard for future enum changes.

ASSERT_UNUSED.

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:76
> +    UNUSED_PARAM(adoptTag);
>      ASSERT(adoptTag == AdoptWK); // Guard for future enum changes.

Ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2799
>  {
> +    UNUSED_PARAM(frame);
>      ASSERT(frame->isMainFrame());

Ditto.

[Benjamin Poulain]

> > Source/WebCore/ChangeLog:11
> > +        No new tests.
> 
> Need to mention why this patch doesn't need to have test case.

Not necessarily, this patch is completely trivial. But do not put "No new tests." if you don't have an explanation for it.

[Byungwoo Lee]

Comment on attachment 165274 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        No new tests.
> 
> Need to mention why this patch doesn't need to have test case.

Ok~ I'll add more detail.

>> Source/JavaScriptCore/heap/MachineStackMarker.cpp:101
>> +    UNUSED_PARAM(signo);
> 
> You should remove the parameter name in this case.

Ok~ Is it ok to remove the name? or should it be changed to (int /*signo*/) ?

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:306
>> +    UNUSED_PARAM(pc);
> 
> Can't you remove the parameter entirely?
> If not, remove the name.

Ok~ I'll remove the name.

>> Source/JavaScriptCore/runtime/DatePrototype.cpp:201
>>      UDateFormatStyle timeStyle = (format != LocaleDate ? UDAT_LONG : UDAT_NONE);
> 
> Remove the parameter name.

Ok~ I'll remove it.

>> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:273
>>      ASSERT(mimeType == "image/png"); // Only PNG output is supported for now.
> 
> This should be ASSERT_UNUSED

Ok~ I'll apply it. Thanks

>> Source/WebKit2/Platform/CoreIPC/Connection.cpp:430
>> +    UNUSED_PARAM(syncRequestID);
> 
> Remove syncRequestID.

I'll use ASSERT_UNUSED() as you guided. (It is used at ASSERT() at 452 line)

[Byungwoo Lee]

(In reply to comment #10)
> > > Source/WebCore/ChangeLog:11
> > > +        No new tests.
> > 
> > Need to mention why this patch doesn't need to have test case.
> 
> Not necessarily, this patch is completely trivial. But do not put "No new tests." if you don't have an explanation for it.

Ok~ I'll remove it :)

[Benjamin Poulain]

> Ok~ Is it ok to remove the name? or should it be changed to (int /*signo*/) ?

When it is obvious, no need to have anything.

[Byungwoo Lee]

(In reply to comment #13)
> > Ok~ Is it ok to remove the name? or should it be changed to (int /*signo*/) ?
> 
> When it is obvious, no need to have anything.

Ok~ Thank you :)

[Byungwoo Lee]

Created attachment 165277 [details]
Patch

[Byungwoo Lee]

Comment on attachment 165277 [details]
Patch

Thank you for your reviews. :)

[WebKit Review Bot]

Comment on attachment 165277 [details]
Patch

Clearing flags on attachment: 165277

Committed r129319: <http://trac.webkit.org/changeset/129319>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.