Bug 45384

Summary: Fix ignoring return value warning in case of gcc 4.4.4
Product: WebKit Reporter: Peter Varga <pvarga>
Component: New BugsAssignee: Peter Varga <pvarga>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, loki, ossy, zherczeg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on: 52585    
Bug Blocks: 43191    
Attachments:
Description Flags
proposed patch
none
Proposed patch jarred: review-

Description Peter Varga 2010-09-08 05:33:51 PDT
I got this warning: "ignoring return value of 'size_t fwrite(const void*, size_t, size_t, FILE*)', declared with attribute warn_unused_result". We shouldn't ignore return value of fwrite because it has __attribute__((warn_unused_result)).

Patch is coming soon.
Comment 1 Csaba Osztrogonác 2010-09-08 05:36:53 PDT
Nice catch. I didn't run into this, because we use gcc-4.3.2 in Debian Lenny.
Comment 2 Peter Varga 2010-09-08 05:49:16 PDT
Created attachment 66885 [details]
proposed patch
Comment 3 Csaba Osztrogonác 2010-09-08 05:51:22 PDT
(In reply to comment #2)
> Created an attachment (id=66885) [details]
> proposed patch

LGTM.
Comment 4 Kenneth Rohde Christiansen 2010-09-08 06:11:26 PDT
Comment on attachment 66885 [details]
proposed patch

I would at least add a comment in the ImageDiff case
Comment 5 Gabor Loki 2010-09-08 06:20:38 PDT
I think it would be better using a LIKELY macro instead of an empty if in ImageDiff. It looks better and have the same effect.
Comment 6 Zoltan Herczeg 2010-09-08 06:49:14 PDT
A propose a generic function for GCC:

static inline __attribute__((always_inline)) int ignoreIntResult(int x) { return x; }

And a define for other platforms:

#define ignoreIntResult(x) x

It would make clear what the developer wants.
Comment 7 Peter Varga 2010-09-08 07:32:14 PDT
(In reply to comment #5)
> I think it would be better using a LIKELY macro instead of an empty if in ImageDiff. It looks better and have the same effect.

I think the LIKELY macro can be misleading.

(In reply to comment #6)
> A propose a generic function for GCC:
> 
> static inline __attribute__((always_inline)) int ignoreIntResult(int x) { return x; }
> 
> And a define for other platforms:
> 
> #define ignoreIntResult(x) x
> 
> It would make clear what the developer wants.

To create a new macro for only one case I think it is unnecessary. If there are more then one function with unused result then the idea of use a macro is reasonable.

Currently the "empty if" solution is sufficient.
Comment 8 Csaba Osztrogonác 2010-09-08 08:03:49 PDT
(In reply to comment #5)
> I think it would be better using a LIKELY macro instead of an empty if in ImageDiff. It looks better and have the same effect.

LIKELY macro would be better, but IMHO it wouldn't look better. :)
We normally use LIKELY and UNLIKELY macros to give hints to branch 
predictor. To use them for ignoring a return value is misunderstandable.
But I agree, it would be more optimal when you build with -O0, because
GCC won't generate useless code for this construction.

I think a new IGNORE_RESULT macro would be useful which uses 
LIKELY. But using LIKELY directly is misunderstandable.

(In reply to comment #6)
> A propose a generic function for GCC:
> 
> static inline __attribute__((always_inline)) int ignoreIntResult(int x) { return x; }
> 
> And a define for other platforms:
> 
> #define ignoreIntResult(x) x
> 
> It would make clear what the developer wants.

It is a good idea, but isn't perfect. For example fwrite has size_t 
type, other functions can have other type. What do you think about 
an ignoreResult template similar to in wtf/Unused.h?

I mean a construction like this:

template<typename T>
inline void ignoreResult(T& x) { return x; }
#define IGNORE_RESULT(result) UNUSED_PARAM(ignoreResult(result))
Comment 9 Csaba Osztrogonác 2010-09-08 08:16:24 PDT
Comment on attachment 66885 [details]
proposed patch

Landed in http://trac.webkit.org/changeset/66984 with comment which Kenneth proposed.
I thought it should be landed as soon as possible not to bother developers with build break.

But I'm open to discuss about the best and general solution for this bug, so I leave the bug open.
Comment 10 Alexey Proskuryakov 2010-09-08 09:46:29 PDT
I think that the most common (and simple) method to ignore this warning is to cast to void:

+                    if (fwrite(data.constData(), 1, data.length(), stdout)) {}
-                    (void)fwrite(data.constData(), 1, data.length(), stdout);
Comment 11 Csaba Osztrogonác 2010-09-08 09:49:05 PDT
(In reply to comment #10)
> I think that the most common (and simple) method to ignore this warning is to cast to void:
> 
> +                    if (fwrite(data.constData(), 1, data.length(), stdout)) {}
> -                    (void)fwrite(data.constData(), 1, data.length(), stdout);

We first tried this idea, but unfortunately it doesn't work when
__attribute__((warn_unused_result)) is used. :/

Details can be found here: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=25509
Comment 12 Alexey Proskuryakov 2010-09-08 10:38:59 PDT
Thanks, I didn't know about that. So, it is considered to be a glibc bug that fwrite is annotated with warn_unused_result.
Comment 13 Jarred Nicholls 2011-01-17 13:10:34 PST
Created attachment 79208 [details]
Proposed patch

Check the return value instead of disabling a gcc warning flag, to benefit all ports.
Comment 14 Jarred Nicholls 2011-01-17 13:12:24 PST
Comment on attachment 79208 [details]
Proposed patch

wrong bug, sorry
Comment 15 Csaba Osztrogonác 2014-12-05 06:55:31 PST
There is no Qt, no need for any discussion about it.