Ticket #1897 (accepted defect) — at Version 12

Opened 10 years ago

Last modified 10 years ago

Build breaks on ignored return values

Reported by: metux Owned by: metux
Priority: blocker Milestone: 4.7.2
Component: mc-core Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: Votes for changeset: slavazanko

Description (last modified by metux) (diff)

glibc has a lot of functions whose return values should not be ignored. this produces a lot of warnings, which cause the build to break when using -Werror (which *really* should be standard for any production build ;-p):

background.c:446: error: ignoring return value of 'write', declared with attribute warn_unused_result

(setting the priority to blocker, since it breaks any serious build)

branch:1897_libc_return_values
changeset:b396e0e959179f85e2a10c6d2cffb18879025e0b

Change History

comment:1 follow-up: ↓ 2 Changed 10 years ago by andrew_b

Branch: 1897_libc_return_values.

-       fscanf (f, ";\n");
+       if (fscanf (f, ";\n"));

Where you've seen such way? The widely used way to suppress warings of unsed return values is

       (void) fscanf (f, ";\n");

I think we should use a common coding styles in MC codebase.

comment:2 in reply to: ↑ 1 Changed 10 years ago by slyfox

Replying to andrew_b:

Branch: 1897_libc_return_values.

-       fscanf (f, ";\n");
+       if (fscanf (f, ";\n"));

Where you've seen such way?

Agreed, looks ambiguous, odd and (not yet) generates another warning:

warning: suggest braces around empty body in an 'if' statement [-Wempty-body]

The widely used way to suppress warings of unsed return values is

       (void) fscanf (f, ";\n");

I think we should use a common coding styles in MC codebase.

Agreed.

comment:3 Changed 10 years ago by metux

  • Status changed from new to accepted
  • Version changed from version not selected to master
  • Description modified (diff)
  • severity changed from no branch to on review

Okay, reworked it. Please review now.

comment:4 follow-up: ↓ 5 Changed 10 years ago by andrew_b

What about

(void) fscanf (f, ";\n");

comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 10 years ago by metux

Replying to andrew_b:

What about

(void) fscanf (f, ";\n");

Doesnt work, still get the warnings with that.
The best way seems to be:

if (ffscanf (f, ";\n")) {}

Changed it this way in the branch.

Please review now. We should get in these fixes (also the #1872 branch) ASAP, since master is still broken.

comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 10 years ago by andrew_b

Replying to metux:

The best way seems to be:

if (ffscanf (f, ";\n")) {}

The best way is not ignore return values. :p

Please review now. We should get in these fixes (also the #1872 branch) ASAP, since master is still broken.

Actually, master is not broken.

comment:7 in reply to: ↑ 6 Changed 10 years ago by metux

Replying to andrew_b:

Replying to metux:

The best way seems to be:

if (ffscanf (f, ";\n")) {}

The best way is not ignore return values. :p

Right. So everybody please review and think carefully
think about whether/how to handle the results, case
by case.

Please review now. We should get in these fixes (also the #1872 branch) ASAP, since master is still broken.

Actually, master is not broken.

It doesnt build in several configs (not just due -Werror!), eg.
--without-edit or --without-vfs totally break.

comment:8 Changed 10 years ago by metux

Okay boys, I've heavily reworked it.
Now the errors are actually handled.

Please review

comment:9 follow-up: ↓ 11 Changed 10 years ago by slavazanko

  • Votes for changeset set to slavazanko

I have commit in branch:

Except this, all looks fine. Enrico, you made big work. Thanks.

comment:10 Changed 10 years ago by slavazanko

  • Milestone changed from 4.7 to 4.7.1

comment:11 in reply to: ↑ 9 Changed 10 years ago by metux

Replying to slavazanko:

I have commit in branch:

Except this, all looks fine. Enrico, you made big work. Thanks.

Sorry, overlooked it when rewriting from just-ignore to actually-handling ;-o

All others folks: please review now, we need to get that in asap ;-p

comment:12 Changed 10 years ago by metux

  • Description modified (diff)

Master had several changes, so rebased it and fixed a few more of these issues.

Note: See TracTickets for help on using tickets.