Bug 79194

Summary: [EFL] Build warning: Fix warn_unused_result warnings
Product: WebKit Reporter: Byungwoo Lee <bw80.lee>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, lucas.de.marchi, rakuco, s.choi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Byungwoo Lee 2012-02-21 22:04:56 PST
Hi,

I met some build warnings on building webkit efl port.


** On release build
    ($ ./Tools/Scripts/build-webkit --efl)
...
[ 49%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/platform/ContextMenu.cpp.o
/WebKit/Source/WebCore/platform/ContextMenu.cpp: In function const WebCore::ContextMenuItem* WebCore::findItemWithAction(unsigned int, const WTF::Vector<WebCore::ContextMenuItem, 0u>&):
/WebKit/Source/WebCore/platform/ContextMenu.cpp:41: warning: comparison between signed and unsigned integer expressions
...
[ 96%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/platform/efl/SharedTimerEfl.cpp.o
/WebKit/Source/WebCore/platform/efl/SharedBufferEfl.cpp: In static member function static WTF::PassRefPtr<WebCore::SharedBuffer> WebCore::SharedBuffer::createWithContentsOfFile(const WTF::String&):
/WebKit/Source/WebCore/platform/efl/SharedBufferEfl.cpp:64: warning: ignoring return value of size_t fread(void*, size_t, size_t, FILE*), declared with attribute warn_unused_result
...
[100%] Building CXX object Source/WebKit/CMakeFiles/ewebkit.dir/efl/ewk/ewk_window_features.cpp.o
/WebKit/Source/WebKit/efl/ewk/ewk_view_single.cpp: In function void _ewk_view_screen_move(uint32_t*, size_t, size_t, size_t, size_t, size_t, size_t, size_t):
/WebKit/Source/WebKit/efl/ewk/ewk_view_single.cpp:109: warning: comparison between signed and unsigned integer expressions
/WebKit/Source/WebKit/efl/ewk/ewk_view_single.cpp:116: warning: comparison between signed and unsigned integer expressions
...
[100%] Building C object Tools/EWebLauncher/CMakeFiles/bin/EWebLauncher.dir/main.c.o
/WebKit/Tools/DumpRenderTree/efl/ImageDiff.cpp: In function void printImage(Evas_Object*):
/WebKit/Tools/DumpRenderTree/efl/ImageDiff.cpp:213: warning: ignoring return value of ssize_t write(int, const void*, size_t), declared with attribute warn_unused_result
...

** On build for DRT test
    ($ ./Tools/Scripts/build-webkit --efl --touch-events --touch-icon-loading \
                                    --blob --device-orientation --fullscreen-api \
                                    --geolocation --notifications --orientation-events \
                                    --video --workers --shared-workers --request-animation-frame \
                                    --page-visibility-api --cmakearg="-DSHARED_CORE=ON")
...
[ 74%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/bindings/js/JSEventCustom.cpp.o
/WebKit/Source/WebCore/bindings/js/JSDocumentCustom.cpp: In member function JSC::JSValue WebCore::JSDocument::createTouchList(JSC::ExecState*):
/WebKit/Source/WebCore/bindings/js/JSDocumentCustom.cpp:122: warning: comparison between signed and unsigned integer expressions
...
[ 96%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/platform/efl/SharedBufferEfl.cpp.o
/WebKit/Source/WebCore/platform/efl/SharedBufferEfl.cpp: In static member function static WTF::PassRefPtr<WebCore::SharedBuffer> WebCore::SharedBuffer::createWithContentsOfFile(const WTF::String&):
/WebKit/Source/WebCore/platform/efl/SharedBufferEfl.cpp:64: warning: ignoring return value of size_t fread(void*, size_t, size_t, FILE*), declared with attribute warn_unused_result
...
Comment 1 Byungwoo Lee 2012-02-23 03:04:55 PST
Changed title to '[EFL] Build warning: Fix warn_unused_result warnings'. And re-scoping this bug only for warn_unused_result warning.

** On release build
    ($ ./Tools/Scripts/build-webkit --efl)
...
[100%] Building C object Tools/EWebLauncher/CMakeFiles/bin/EWebLauncher.dir/main.c.o
/WebKit/Tools/DumpRenderTree/efl/ImageDiff.cpp: In function void printImage(Evas_Object*):
/WebKit/Tools/DumpRenderTree/efl/ImageDiff.cpp:213: warning: ignoring return value of ssize_t write(int, const void*, size_t), declared with attribute warn_unused_result
...


** On build for DRT test
    ($ ./Tools/Scripts/build-webkit --efl --touch-events --touch-icon-loading \
                                    --blob --device-orientation --fullscreen-api \
                                    --geolocation --notifications --orientation-events \
                                    --video --workers --shared-workers --request-animation-frame \
                                    --page-visibility-api --cmakearg="-DSHARED_CORE=ON")
...
[ 96%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/platform/efl/SharedBufferEfl.cpp.o
/WebKit/Source/WebCore/platform/efl/SharedBufferEfl.cpp: In static member function static WTF::PassRefPtr<WebCore::SharedBuffer> WebCore::SharedBuffer::createWithContentsOfFile(const WTF::String&):
/WebKit/Source/WebCore/platform/efl/SharedBufferEfl.cpp:64: warning: ignoring return value of size_t fread(void*, size_t, size_t, FILE*), declared with attribute warn_unused_result
...
Comment 2 Byungwoo Lee 2012-03-05 18:46:38 PST
Created attachment 130263 [details]
Patch
Comment 3 Raphael Kubo da Costa (:rakuco) 2012-03-05 19:13:00 PST
Comment on attachment 130263 [details]
Patch

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

> Source/WebCore/platform/efl/SharedBufferEfl.cpp:71
> -    fread(result->m_buffer.data(), 1, fileStat.st_size, file);
> +    if (static_cast<unsigned>(fileStat.st_size) != fread(result->m_buffer.data(), 1, fileStat.st_size, file)) {
> +        fclose(file);
> +        return 0;
> +    }
> +
>      fclose(file);
>  
>      return result.release();

How about

  const size_t freadResult = fread(...);
  fclose(file);

  const bool readSuccessful = (freadResult == static_cast<size_t>(fileStat.st_size));

  return readSucessful ? result.relase() : 0;

> Tools/DumpRenderTree/efl/ImageDiff.cpp:218
> +                } while ((bytesWritten += count) < bytesRead);

Personally, I find (bytesWritten += count) here weird to read (no pun intended :-). How about moving the increase operation inside the do-while loop?
Comment 4 Byungwoo Lee 2012-03-05 21:00:26 PST
Thanks for your comment.

> How about
> 
>   const size_t freadResult = fread(...);
>   fclose(file);
> 
>   const bool readSuccessful = (freadResult == static_cast<size_t>(fileStat.st_size));
> 
>   return readSucessful ? result.relase() : 0;
This looks better for me also, and I think 'readSuccessful' is redundant.
How about the below?

-    fread(result->m_buffer.data(), 1, fileStat.st_size, file);
+    const size_t bytesRead = fread(result->m_buffer.data(), 1, fileStat.st_size, file);
     fclose(file);
 
-    return result.release();
+    return bytesRead == static_cast<unsigned>(fileStat.st_size) ? result.release() : 0;




> > Tools/DumpRenderTree/efl/ImageDiff.cpp:218
> > +                } while ((bytesWritten += count) < bytesRead);
> 
> Personally, I find (bytesWritten += count) here weird to read (no pun intended :-). How about moving the increase operation inside the do-while loop?
Ok~ I'll fix as below
+                    bytesWritten += count;
+                } while (bytesWritten < bytesRead);
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-03-06 04:42:53 PST
(In reply to comment #4)
> How about the below?
> 
> -    fread(result->m_buffer.data(), 1, fileStat.st_size, file);
> +    const size_t bytesRead = fread(result->m_buffer.data(), 1, fileStat.st_size, file);
>      fclose(file);
> 
> -    return result.release();
> +    return bytesRead == static_cast<unsigned>(fileStat.st_size) ? result.release() : 0;

That's OK too.
Comment 6 Byungwoo Lee 2012-03-06 05:05:35 PST
Created attachment 130358 [details]
Patch
Comment 7 Filip Pizlo 2012-03-07 00:06:03 PST
Comment on attachment 130358 [details]
Patch

r=me.
Comment 8 WebKit Review Bot 2012-03-07 01:24:22 PST
Comment on attachment 130358 [details]
Patch

Clearing flags on attachment: 130358

Committed r110035: <http://trac.webkit.org/changeset/110035>
Comment 9 WebKit Review Bot 2012-03-07 01:24:26 PST
All reviewed patches have been landed.  Closing bug.