ui_curses : return values of mvaddstr / mvaddch

[Xavier Chantry]

clang spotted that the return value of these two functions are never
checked in ui_curses.c
Is this something to worry about ? It's not obvious how to best handle
potential errors with these functions.

ui_curses.c:329:3: warning: expression result unused [-Wunused-value]
mvaddstr(row, col, print_buffer);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Gregory Petrosyan]

On Sun, May 23, 2010 at 8:37 PM, Xavier Chantry

Post by Xavier Chantry
clang spotted that the return value of these two functions are never
checked in ui_curses.c
Is this something to worry about ? It's not obvious how to best handle
potential errors with these functions.
ui_curses.c:329:3: warning: expression result unused [-Wunused-value]
mvaddstr(row, col, print_buffer);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I think we can more or less ignore warnings like this till there are
much more serious issues in cmus' codebase.

But if you have a patch ready – you know, please don't mind to share it :-)

Gregory

[Xavier Chantry]

On Sun, May 23, 2010 at 6:55 PM, Gregory Petrosyan

Post by Gregory Petrosyan
On Sun, May 23, 2010 at 8:37 PM, Xavier Chantry

Post by Xavier Chantry
clang spotted that the return value of these two functions are never
checked in ui_curses.c
Is this something to worry about ? It's not obvious how to best handle
potential errors with these functions.
   ui_curses.c:329:3: warning: expression result unused [-Wunused-value]
                   mvaddstr(row, col, print_buffer);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I think we can more or less ignore warnings like this till there are
much more serious issues in cmus' codebase.
But if you have a patch ready – you know, please don't mind to share it :-)

I can believe there are more serious issues, this is as harmless as possible.
It's just that when I was initially debugging the term_title
segfault/corruption, I wanted to rule out any non-checked return
value, so I added an error_msg in each failure case :
error_msg("mvaddstr failed");
But if mvaddstr only fails when things are really bad, I was thinking
error_msg could fail as well or even make things worse.
If you don't think this is useless, I will send that patch too.

[Gregory Petrosyan]

On Sun, May 23, 2010 at 9:05 PM, Xavier Chantry

Post by Xavier Chantry
On Sun, May 23, 2010 at 6:55 PM, Gregory Petrosyan

Post by Gregory Petrosyan
On Sun, May 23, 2010 at 8:37 PM, Xavier Chantry

Post by Xavier Chantry
clang spotted that the return value of these two functions are never
checked in ui_curses.c
Is this something to worry about ? It's not obvious how to best handle
potential errors with these functions.
ui_curses.c:329:3: warning: expression result unused [-Wunused-value]
mvaddstr(row, col, print_buffer);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I think we can more or less ignore warnings like this till there are
much more serious issues in cmus' codebase.
But if you have a patch ready – you know, please don't mind to share it :-)

I can believe there are more serious issues, this is as harmless as possible.
It's just that when I was initially debugging the term_title
segfault/corruption, I wanted to rule out any non-checked return
error_msg("mvaddstr failed");
But if mvaddstr only fails when things are really bad, I was thinking
error_msg could fail as well or even make things worse.
If you don't think this is useless, I will send that patch too.

I vote for sticking with the status quo, unless chances that
mvaddstr() fail are not zero *and* we can do something sensible about
it.

Gregory

[Johannes Weißl]

Hello Xavier,

Post by Xavier Chantry
On Sun, May 23, 2010 at 6:55 PM, Gregory Petrosyan

Post by Gregory Petrosyan
On Sun, May 23, 2010 at 8:37 PM, Xavier Chantry

Post by Xavier Chantry
clang spotted that the return value of these two functions are never
checked in ui_curses.c
Is this something to worry about ? It's not obvious how to best handle
potential errors with these functions.
   ui_curses.c:329:3: warning: expression result unused [-Wunused-value]
                   mvaddstr(row, col, print_buffer);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I think we can more or less ignore warnings like this till there are
much more serious issues in cmus' codebase.
But if you have a patch ready – you know, please don't mind to share it :-)

I can believe there are more serious issues, this is as harmless as possible.
It's just that when I was initially debugging the term_title
segfault/corruption, I wanted to rule out any non-checked return
error_msg("mvaddstr failed");
But if mvaddstr only fails when things are really bad, I was thinking
error_msg could fail as well or even make things worse.
If you don't think this is useless, I will send that patch too.

I found this post while trying to fix the warning myself! I think I
have a pretty good solution to fix the warning in clang and
(potentially) other/future compiler versions! Patch is attached.

Now clang compiles without warnings...


Greetings,
Johannes

[Gregory Petrosyan]

- mvaddch(row, tree_win_w, ACS_VLINE);
+ (void) mvaddch(row, tree_win_w, ACS_VLINE);

Maybe it's better to introduce helper macro, like RESULT_UNUSED?
IMHO "(void) mvaddch(row, tree_win_w, ACS_VLINE)" looks a bit strange in the
code as-is.

Gregory

[Johannes Weißl]

Post by Gregory Petrosyan

- mvaddch(row, tree_win_w, ACS_VLINE);
+ (void) mvaddch(row, tree_win_w, ACS_VLINE);

Maybe it's better to introduce helper macro, like RESULT_UNUSED?
IMHO "(void) mvaddch(row, tree_win_w, ACS_VLINE)" looks a bit strange in the
code as-is.

Hmm, I'm not sure... the main reason for those macros is that they are
compiler-dependant (= non-standard). The (void) trick is standard and
relatively common [1,2]. For me

(void) foo();

looks clearer (because of syntax-highlighting and shortness) than

RESULT_UNUSED foo();

On the other hand, I don't really care, so you can decide :-).


[1] Suggested by gcc manual (under -Wunused-value):
http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
[2] Suggested by OpenOffice coding guide:
http://wiki.services.openoffice.org/wiki/Writing_warning-free_code#Unused_parameters


Johannes

[Gregory Petrosyan]

Post by Johannes Weißl

Post by Gregory Petrosyan

- mvaddch(row, tree_win_w, ACS_VLINE);
+ (void) mvaddch(row, tree_win_w, ACS_VLINE);

Maybe it's better to introduce helper macro, like RESULT_UNUSED?
IMHO "(void) mvaddch(row, tree_win_w, ACS_VLINE)" looks a bit strange in the
code as-is.

Hmm, I'm not sure... the main reason for those macros is that they are
compiler-dependant (= non-standard). The (void) trick is standard and
relatively common [1,2]. For me
(void) foo();
looks clearer (because of syntax-highlighting and shortness) than
RESULT_UNUSED foo();
On the other hand, I don't really care, so you can decide :-).

Thinking about this one more time, I tend to agree with you: (void) is good
fit here. Merged to master, thanks!

Gregory