Fix ignoring return value warning in case of gcc 4.4.4

• Component: New Bugs
• Status: RESOLVED FIXED

[Peter Varga]

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.

[Csaba Osztrogonác]

Nice catch. I didn't run into this, because we use gcc-4.3.2 in Debian Lenny.

[Peter Varga]

Created attachment 66885 [details]
proposed patch

[Csaba Osztrogonác]

(In reply to comment #2)
> Created an attachment (id=66885) [details]
> proposed patch

LGTM.

[Kenneth Rohde Christiansen]

Comment on attachment 66885 [details]
proposed patch

I would at least add a comment in the ImageDiff case

[Gabor Loki]

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.

[Zoltan Herczeg]

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.

[Peter Varga]

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

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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.

[Alexey Proskuryakov]

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

[Csaba Osztrogonác]

(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

[Alexey Proskuryakov]

Thanks, I didn't know about that. So, it is considered to be a glibc bug that fwrite is annotated with warn_unused_result.

[Jarred Nicholls]

Created attachment 79208 [details]
Proposed patch

Check the return value instead of disabling a gcc warning flag, to benefit all ports.

[Jarred Nicholls]

Comment on attachment 79208 [details]
Proposed patch

wrong bug, sorry

[Csaba Osztrogonác]

There is no Qt, no need for any discussion about it.