Bug 58727

Created attachment 98546 [details] [review]
Fix warn_unused_result warnings from gcc

To fix the warn_unused_result warnings from gcc, assign the return value to a local variable and then `(void) varname;` to avoid "unused" warnings.

I've referred to the approach followed in the discussion mentioned at the below link.
URL : http://www.postgresql.org/message-id/514B5688.8090506@dunslane.net

I'm not a machine with compilers right now to test, but you cast the function-call to void rather than assigning to a temp-variable? Instead of:

int unused;
unused=some_function_with_attribute_wur(foo);
(void) unused;

Just:

(void) some_function_with_attribute_wur(foo);

Oh god those gnu-idiots!

I tried it and in fact the multi-line workaround is the only thing that 
works. Or a wrapper function. Compile the following with -Wall:

int fn () __attribute__ ((warn_unused_result));

int used(int i) { return i; }

int main(int argc, const char** argv)
{
   int a, b;
   fn(); /* produces error! */
   (void)fn(); /* produces error! This used to work!*/
   a = fn(); /* produces warning that a is unused */
   b = fn(); (void)b; /* this works */
   used(fn()); /* this also works */
   return 0;
}


On 05/06/2014 07:20 AM, bugzilla-daemon@freedesktop.org wrote:
> *Comment # 2 <https://bugs.freedesktop.org/show_bug.cgi?id=58727#c2> on
> bug 58727 <https://bugs.freedesktop.org/show_bug.cgi?id=58727> from
> Daniel Macks <mailto:dmacks@netspace.org> *
>
> I'm not a machine with compilers right now to test, but you cast the
> function-call to void rather than assigning to a temp-variable? Instead of:
>
> int unused;
> unused=some_function_with_attribute_wur(foo);
> (void) unused;
>
> Just:
>
> (void) some_function_with_attribute_wur(foo);
>
> ------------------------------------------------------------------------
> You are receiving this mail because:
>
>   * You are watching the QA Contact of the bug.
>

(In reply to comment #3)
> Oh god those gnu-idiots!
> 
> I tried it and in fact the multi-line workaround is the only thing that 
> works. Or a wrapper function. Compile the following with -Wall:
> 
> int fn () __attribute__ ((warn_unused_result));
> 
> int used(int i) { return i; }
> 
> int main(int argc, const char** argv)
> {
>    int a, b;
>    fn(); /* produces error! */
>    (void)fn(); /* produces error! This used to work!*/
>    a = fn(); /* produces warning that a is unused */
>    b = fn(); (void)b; /* this works */
>    used(fn()); /* this also works */
>    return 0;
> }
> 

Agreed. But in the wrapper function approach, we will have to change the declarations of all the cairo functions, for which the return values are really not used anywhere. 

> 
> On 05/06/2014 07:20 AM, bugzilla-daemon@freedesktop.org wrote:
> > *Comment # 2 <https://bugs.freedesktop.org/show_bug.cgi?id=58727#c2> on
> > bug 58727 <https://bugs.freedesktop.org/show_bug.cgi?id=58727> from
> > Daniel Macks <mailto:dmacks@netspace.org> *
> >
> > I'm not a machine with compilers right now to test, but you cast the
> > function-call to void rather than assigning to a temp-variable? Instead of:
> >
> > int unused;
> > unused=some_function_with_attribute_wur(foo);
> > (void) unused;
> >
> > Just:
> >
> > (void) some_function_with_attribute_wur(foo);
> >
> > ------------------------------------------------------------------------
> > You are receiving this mail because:
> >
> >   * You are watching the QA Contact of the bug.
> >

Thinking slightly more philosophically, do those functions really need to be (warn_unused_result)? Or, if it's important not to ignore the result, why are we ignoring the result?

I think the complaint is about strtol(), which is being used apparently 
to find the other end of the numeric value (returned in the char** arg) 
while the number is ignored, thus causing this error.

It does seem that nobody is going to "accidentally" ignore the result of 
strtol. The warning really should be on functions returning things you 
*must* do something with, for instance malloc() where if you don't free 
the returned value you have a memory leak.

And they really should restore the (void)fn() work-around, which is 
readable.

On 05/07/2014 07:34 AM, bugzilla-daemon@freedesktop.org wrote:
> *Comment # 5 <https://bugs.freedesktop.org/show_bug.cgi?id=58727#c5> on
> bug 58727 <https://bugs.freedesktop.org/show_bug.cgi?id=58727> from
> Daniel Macks <mailto:dmacks@netspace.org> *
>
> Thinking slightly more philosophically, do those functions really need to be
> (warn_unused_result)? Or, if it's important not to ignore the result, why are
> we ignoring the result?
>
> ------------------------------------------------------------------------
> You are receiving this mail because:
>
>   * You are watching the QA Contact of the bug.
>

(In reply to comment #6)
> I think the complaint is about strtol(), which is being used apparently 
> to find the other end of the numeric value (returned in the char** arg) 
> while the number is ignored, thus causing this error.
> 
> It does seem that nobody is going to "accidentally" ignore the result of 
> strtol. The warning really should be on functions returning things you 
> *must* do something with, for instance malloc() where if you don't free 
> the returned value you have a memory leak.
> 
> And they really should restore the (void)fn() work-around, which is 
> readable.

In some of the cases (for ex: _cairo_polygon_add_external_edge()), the return values of the callee are used by the caller (for ex: ln. no. 63 and 177 in cairo-path-fill.c file), while in other cases it is ignored for some reasons.
The warnings are thrown in these scenarios. In such a case, I fell like we can use the approach provided by Sergey Shandar, @ http://stackoverflow.com/questions/3614691/casting-to-void-doesnt-remove-warn-unused-result-error

Bill, 
Could you please share your opinion on this?

> 
> On 05/07/2014 07:34 AM, bugzilla-daemon@freedesktop.org wrote:
> > *Comment # 5 <https://bugs.freedesktop.org/show_bug.cgi?id=58727#c5> on
> > bug 58727 <https://bugs.freedesktop.org/show_bug.cgi?id=58727> from
> > Daniel Macks <mailto:dmacks@netspace.org> *
> >
> > Thinking slightly more philosophically, do those functions really need to be
> > (warn_unused_result)? Or, if it's important not to ignore the result, why are
> > we ignoring the result?
> >
> > ------------------------------------------------------------------------
> > You are receiving this mail because:
> >
> >   * You are watching the QA Contact of the bug.
> >

On 05/12/2014 11:28 PM, bugzilla-daemon@freedesktop.org wrote:
> *Comment # 7 <https://bugs.freedesktop.org/show_bug.cgi?id=58727#c7> on
> bug 58727 <https://bugs.freedesktop.org/show_bug.cgi?id=58727> from Ravi
> Nanjundappa <mailto:nravi.n@samsung.com> *
>
> (In reply tocomment #6  <show_bug.cgi?id=58727#c6>)
>> I think the complaint is about strtol(), which is being used apparently
>> to find the other end of the numeric value (returned in the char** arg)
>> while the number is ignored, thus causing this error.
>>
>> It does seem that nobody is going to "accidentally" ignore the result of
>> strtol. The warning really should be on functions returning things you
>> *must* do something with, for instance malloc() where if you don't free
>> the returned value you have a memory leak.
>>
>> And they really should restore the (void)fn() work-around, which is
>> readable.
>
> In some of the cases (for ex: _cairo_polygon_add_external_edge()), the return
> values of the callee are used by the caller (for ex: ln. no. 63 and 177 in
> cairo-path-fill.c file), while in other cases it is ignored for some reasons.
> The warnings are thrown in these scenarios. In such a case, I fell like we can
> use the approach provided by Sergey Shandar, @
> http://stackoverflow.com/questions/3614691/casting-to-void-doesnt-remove-warn-unused-result-error
>
> Bill,
> Could you please share your opinion on this?

My tests just confirm that the ridiculous workaround described in the 
link (of using a temporary and casting *that* to void) is required by 
gcc, instead of the more sensible workaround of casting the function 
call to void.

I have no opinion on whether removing the warn-unused-result attribute 
from that function, or adding the workaround, or figuring out if the 
code should in fact should be paying attention to the result.

Created attachment 99012 [details] [review]
Fix warn_unused_result warnings from gcc - 2nd approach

While going through the source code of cairoint.h file, I found one more
simpler approach for handling the warnings.
Attached patch has the changes with this approach. ( except the warning
introduced due to usage of strtol). 
This looks somewhat more cleaner.
Please have a look at the patch and provide your inputs on this.

We can adopt this or the earlier one, depending on the community's opinion.

Created attachment 99312 [details] [review]
Fix warn_unused_result warnings from gcc - 2nd approach

Updated the patch with the proper commit msg. 
Build results are verified.

Thanks, applied to trunk.

Thank you Bryce. :)