Bug 79194 – [EFL] Build warning: Fix warn_unused_result warnings

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Byungwoo Lee]

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
...

[Byungwoo Lee]

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
...

[Byungwoo Lee]

Created attachment 130263 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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?

[Byungwoo Lee]

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);

[Raphael Kubo da Costa (:rakuco)]

(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.

[Byungwoo Lee]

Created attachment 130358 [details]
Patch

[Filip Pizlo]

Comment on attachment 130358 [details]
Patch

r=me.

[WebKit Review Bot]

Comment on attachment 130358 [details]
Patch

Clearing flags on attachment: 130358

Committed r110035: <http://trac.webkit.org/changeset/110035>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.