Bug 58727 - warn_unused_result noise
Summary: warn_unused_result noise
Status: RESOLVED FIXED
Alias: None
Product: cairo
Classification: Unclassified
Component: general (show other bugs)
Version: 1.12.8
Hardware: Other All
: medium normal
Assignee: Chris Wilson
QA Contact: cairo-bugs mailing list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: cairo-1.14
  Show dependency treegraph
 
Reported: 2012-12-24 19:46 UTC by Daniel Macks
Modified: 2014-06-06 03:47 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Fix warn_unused_result warnings from gcc (7.43 KB, patch)
2014-05-06 09:35 UTC, Ravi Nanjundappa
Details | Splinter Review
Fix warn_unused_result warnings from gcc - 2nd approach (2.19 KB, patch)
2014-05-14 07:00 UTC, Ravi Nanjundappa
Details | Splinter Review
Fix warn_unused_result warnings from gcc - 2nd approach (2.38 KB, patch)
2014-05-19 09:31 UTC, Ravi Nanjundappa
Details | Splinter Review

Description Daniel Macks 2012-12-24 19:46:46 UTC
Building cairo-1.12.8 on OS X 10.6 against pixman-0.28.2, I get a bunch of warnings about ignored return-codes of functions that assert that their return should not be ignored:

cairo-path-stroke-polygon.c: In function 'add_caps':
cairo-path-stroke-polygon.c:944: warning: ignoring return value of '_cairo_polygon_add_contour', declared with attribute warn_unused_result
cairo-path-stroke-polygon.c:961: warning: ignoring return value of '_cairo_polygon_add_contour', declared with attribute warn_unused_result
cairo-path-stroke-polygon.c:976: warning: ignoring return value of '_cairo_polygon_add_contour', declared with attribute warn_unused_result
cairo-path-stroke-polygon.c:981: warning: ignoring return value of '_cairo_polygon_add_contour', declared with attribute warn_unused_result
cairo-path-stroke-polygon.c: In function 'close_path':
cairo-path-stroke-polygon.c:1226: warning: ignoring return value of '_cairo_polygon_add_contour', declared with attribute warn_unused_result
cairo-path-stroke-polygon.c:1227: warning: ignoring return value of '_cairo_polygon_add_contour', declared with attribute warn_unused_result

cairo-polygon-intersect.c: In function 'edges_end':
cairo-polygon-intersect.c:1162: warning: ignoring return value of '_cairo_polygon_add_line', declared with attribute warn_unused_result
cairo-polygon-intersect.c:1163: warning: ignoring return value of '_cairo_polygon_add_line', declared with attribute warn_unused_result
cairo-polygon-intersect.c: In function '_cairo_polygon_intersect_with_boxes':
cairo-polygon-intersect.c:1519: warning: ignoring return value of '_cairo_polygon_add_external_edge', declared with attribute warn_unused_result
cairo-polygon-intersect.c:1522: warning: ignoring return value of '_cairo_polygon_add_external_edge', declared with attribute warn_unused_result
cairo-polygon-reduce.c: In function '_cairo_bo_edge_end':
cairo-polygon-reduce.c:1163: warning: ignoring return value of '_cairo_polygon_add_line', declared with attribute warn_unused_result
cairo-polygon-reduce.c:1167: warning: ignoring return value of '_cairo_polygon_add_line', declared with attribute warn_unused_result

cairo-rectangular-scan-converter.c: In function 'generate_row':
cairo-rectangular-scan-converter.c:625: warning: ignoring return value of function declared with attribute warn_unused_result

cairo-surface-observer.c: In function 'sync':
cairo-surface-observer.c:665: warning: ignoring return value of '_cairo_surface_unmap_image', declared with attribute warn_unused_result

Do they need their return-codes handled, or do their prototypes (in cairoint.h I think) need to allow their return-codes to be ignoreable?
Comment 1 Ravi Nanjundappa 2014-05-06 09:35:29 UTC
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
Comment 2 Daniel Macks 2014-05-06 14:20:41 UTC
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);
Comment 3 Bill Spitzak 2014-05-06 18:34:55 UTC
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.
>
Comment 4 Ravi Nanjundappa 2014-05-07 13:06:09 UTC
(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.
> >
Comment 5 Daniel Macks 2014-05-07 14:34:36 UTC
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?
Comment 6 Bill Spitzak 2014-05-07 17:55:40 UTC
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.
>
Comment 7 Ravi Nanjundappa 2014-05-13 06:28:35 UTC
(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.
> >
Comment 8 Bill Spitzak 2014-05-13 19:24:05 UTC
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.
Comment 9 Ravi Nanjundappa 2014-05-14 07:00:14 UTC
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.
Comment 10 Ravi Nanjundappa 2014-05-19 09:31:16 UTC
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.
Comment 11 Bryce Harrington 2014-06-05 23:12:55 UTC
Thanks, applied to trunk.
Comment 12 Ravi Nanjundappa 2014-06-06 03:47:43 UTC
Thank you Bryce. :)


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.