Patch to Add -Wunused-returns

Patch to Add -Wunused-returns

[David E. Weekly]

GCC Folks,

Thanks for your wide response (generally along the lines of "okay, if you
want it - do it!"). I've resurrected Gray Watson's patch
(http://256.com/gray/) and 95% of the credit of this patch belongs to him.
(I obtained his permission to resurrect this patch.) I basically just
slapped his code in there my way and gave it a quick check. Seems to work on
some simple test cases just fine.

Incidentally, when running my diff it became clear that some files are in
CVS that oughtn't be -- i.e., are autogenerated! Here they are:

    fastjar/Makefile.in (made from fastjar/Makefile.am)
    fastjar/configure   (made from configure.in)

Needless to say, having these auto-generated files in CVS makes diffing
annoying. =)

There were also numerous warnings when compiling GCC from GCC 2.96 and XGCC
itself. Is this okay, or is the hope to someday have a smooth, clean build?

Here is a contextual diff for readability.

Cheers,
 -david




Index: gcc/c-common.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/c-common.c,v
retrieving revision 1.279
diff -u -r1.279 c-common.c
--- c-common.c 2001/12/05 23:19:54 1.279
+++ c-common.c 2001/12/07 04:57:44
@@ -1244,6 +1244,26 @@
   return value;
 }

+/* Check to see if a non-void function's return value is ignored. */
+
+void
+check_for_unused_returns (expr)
+     tree expr;
+{
+  tree function;
+
+  /* Make sure our expression is a function. */
+  if (TREE_CODE (expr) != CALL_EXPR ||
+      TREE_TYPE (expr) == void_type_node)
+    return;
+
+  function = TREE_OPERAND (expr, 0);
+
+  if ((TREE_CODE (function) == ADDR_EXPR) &&
+      (TREE_CODE (TREE_OPERAND (function, 0)) == FUNCTION_DECL))
+    warning("return value from function ignored");
+}
+
 /* Return an integer type with BITS bits of precision,
    that is unsigned if UNSIGNEDP is nonzero, otherwise signed.  */

Index: gcc/c-decl.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/c-decl.c,v
retrieving revision 1.281
diff -u -r1.281 c-decl.c
--- c-decl.c 2001/12/05 23:19:55 1.281
+++ c-decl.c 2001/12/07 04:57:45
@@ -419,6 +419,10 @@

 int warn_unknown_pragmas = 0; /* Tri state variable.  */

+/* Warn about unused return values from non-void functions. */
+
+int warn_unused_returns = 0;
+
 /* Warn about comparison of signed and unsigned values.
    If -1, neither -Wsign-compare nor -Wno-sign-compare has been specified.
*/

@@ -765,6 +769,10 @@
     warn_unknown_pragmas = 2;
   else if (!strcmp (p, "-Wno-unknown-pragmas"))
     warn_unknown_pragmas = 0;
+  else if (!strcmp (p, "-Wunused-returns"))
+    warn_unused_returns = 1;
+  else if (!strcmp (p, "-Wno-unused-returns"))
+    warn_unused_returns = 0;
   else if (!strcmp (p, "-Wall"))
     {
       /* We save the value of warn_uninitialized, since if they put
Index: gcc/c-parse.in
===================================================================
RCS file: /cvs/gcc/gcc/gcc/c-parse.in,v
retrieving revision 1.117
diff -u -r1.117 c-parse.in
--- c-parse.in 2001/12/04 22:55:37 1.117
+++ c-parse.in 2001/12/07 04:57:46
@@ -2314,6 +2314,8 @@
  { stmt_count++; $$ = $1; }
  | expr ';'
  { stmt_count++;
+   if (warn_unused_returns)
+     check_for_unused_returns ($1);
    $$ = c_expand_expr_stmt ($1); }
  | c99_block_start select_or_iter_stmt c99_block_end
  { if (flag_isoc99)
Index: gcc/c-tree.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/c-tree.h,v
retrieving revision 1.77
diff -u -r1.77 c-tree.h
--- c-tree.h 2001/12/04 22:55:37 1.77
+++ c-tree.h 2001/12/07 04:57:46
@@ -183,6 +183,7 @@
 extern int  c_decode_option                     PARAMS ((int, char **));
 extern void c_mark_varargs                      PARAMS ((void));
 extern void check_for_loop_decls                PARAMS ((void));
+extern void check_for_unused_returns            PARAMS ((tree));
 extern tree check_identifier                    PARAMS ((tree, tree));
 extern void clear_parm_order                    PARAMS ((void));
 extern int  complete_array_type                 PARAMS ((tree, tree, int));
@@ -361,6 +362,10 @@
 /* Warn about multicharacter constants.  */

 extern int warn_multichar;
+
+/* Warn about unused return values from non-void functions. */
+
+extern int warn_unused_returns;

 /* Nonzero means we are reading code that came from a system header file.
*/

Index: gcc/doc/invoke.texi
===================================================================
RCS file: /cvs/gcc/gcc/gcc/doc/invoke.texi,v
retrieving revision 1.85
diff -u -r1.85 invoke.texi
--- invoke.texi 2001/12/06 11:49:45 1.85
+++ invoke.texi 2001/12/07 04:57:50
@@ -235,7 +235,7 @@
 @item C-only Warning Options
 @gccoptlist{
 -Wbad-function-cast  -Wmissing-prototypes  -Wnested-externs @gol
--Wstrict-prototypes  -Wtraditional}
+-Wstrict-prototypes  -Wtraditional  -Wunused-returns}

 @item Debugging Options
 @xref{Debugging Options,,Options for Debugging Your Program or GCC}.
@@ -2068,6 +2068,14 @@
 In order to get a warning about an unused function parameter, you must
 either specify @samp{-W -Wunused} or separately specify
 @option{-Wunused-parameter}.
+
+@item -Wunused-returns @r{(C only)}
+@opindex Wunused-returns
+Warn whenever the result of a non-void function is implicitly cast away.
+It is not included in the above, because it requires @code{void} casting
+returns of all sorts of regular functions, like @code{close} and
+@code{printf}, whose return value you usually don't care about. This makes
+GCC more like Lint.

 @item -Wuninitialized
 @opindex Wuninitialized

Re: Patch to Add -Wunused-returns

[Fergus Henderson]

On 06-Dec-2001, David E. Weekly <dweekly@legato.com> wrote:
> +++ c-common.c 2001/12/07 04:57:44
...
> +/* Check to see if a non-void function's return value is ignored. */
> +
> +void
> +check_for_unused_returns (expr)
> +     tree expr;
> +{
> +  tree function;
> +
> +  /* Make sure our expression is a function. */
> +  if (TREE_CODE (expr) != CALL_EXPR ||
> +      TREE_TYPE (expr) == void_type_node)
> +    return;
> +
> +  function = TREE_OPERAND (expr, 0);
> +
> +  if ((TREE_CODE (function) == ADDR_EXPR) &&
> +      (TREE_CODE (TREE_OPERAND (function, 0)) == FUNCTION_DECL))
> +    warning("return value from function ignored");

I don't see what the point of the second `if' is.  Surely the same
warning should be issued for calls via function pointers as is issued for
ordinary calls.

So I'd just write the body of this function like this:

  if (TREE_CODE (expr) == CALL_EXPR &&
      TREE_TYPE (expr) == void_type_node)
    warning("return value from function call ignored");

On the other hand, if it is an ordinary function call, then it would be
nice if the warning message included the name of the function.
So maybe it would be better to keep the code you have above,
and change the last three lines to something like this
(completely untested):

  if ((TREE_CODE (function) == ADDR_EXPR) &&
      (TREE_CODE (TREE_OPERAND (function, 0)) == FUNCTION_DECL))
    warning_with_decl(TREE_OPERAND (function, 0),
                      "return value from function `%s' ignored");
  else
    warning("return value from function call ignored");

> Index: gcc/doc/invoke.texi
...
> +@item -Wunused-returns @r{(C only)}
> +@opindex Wunused-returns
> +Warn whenever the result of a non-void function is implicitly cast away.
> +It is not included in the above, because it requires @code{void} casting
> +returns of all sorts of regular functions, like @code{close} and
> +@code{printf}, whose return value you usually don't care about. This makes
> +GCC more like Lint.

I'm not sure that close() is a good example here, since checking the
return value from close() for files opened in output mode is very
important, and it would be bad to encourage people to omit that.

-- 
Fergus Henderson <fjh@cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.

Re: Patch to Add -Wunused-returns

[David E. Weekly]

Fergus,

Thanks for your comments!

>>>
I don't see what the point of the second `if' is.  Surely the same
warning should be issued for calls via function pointers as is issued for
ordinary calls.
<<<

Good point. And I like your idea of printing the function name, too. Shall I
use this code?

>>>
I'm not sure that close() is a good example here, since checking the
return value from close() for files opened in output mode is very
important, and it would be bad to encourage people to omit that.
<<<

Very fair. How about "strcpy()" as another example of where most people
usually (and rightfully) cast away the return value?

-d

Re: Patch to Add -Wunused-returns

[Neil Booth]

David E. Weekly wrote:-

> +/* Check to see if a non-void function's return value is ignored. */

Comments end in full stop followed by 2 spaces in GCC.

> +  /* Make sure our expression is a function. */

> +/* Warn about unused return values from non-void functions. */

Neil.

Re: Patch to Add -Wunused-returns

[Joseph S. Myers]

On Thu, 6 Dec 2001, David E. Weekly wrote:

> Thanks for your wide response (generally along the lines of "okay, if you
> want it - do it!"). I've resurrected Gray Watson's patch
> (http://256.com/gray/) and 95% of the credit of this patch belongs to him.

A current copyright assignment or disclaimer from him will probably be
needed, then.  (The old copy of copyright.list I have only shows

CCCP    Gray Watson     1992-04-21
Disclaims changes to cccp.

CCCP    Antaire Corporation     1992-04-21
Disclaims changes made by Gray Watson to cccp.
info@antaire.com

.)

> (I obtained his permission to resurrect this patch.) I basically just
> slapped his code in there my way and gave it a quick check. Seems to work on
> some simple test cases just fine.

Testcases need to be included in a form suitable for the testsuite (see
existing tests for warnings in gcc.dg).  They should probably test at
least:

* Void value ignored, no warning.
* Non-void value ignored, warning.
* Non-void value cast to void, no warning.

> Incidentally, when running my diff it became clear that some files are in
> CVS that oughtn't be -- i.e., are autogenerated! Here they are:
> 
>     fastjar/Makefile.in (made from fastjar/Makefile.am)
>     fastjar/configure   (made from configure.in)
> 
> Needless to say, having these auto-generated files in CVS makes diffing
> annoying. =)

You should use contrib/gcc_update --touch to avoid files getting
spuriously rebuilt.  These files need to be in CVS for just running
configure and building the compiler from CVS to work - and there are
strong restrictions on the suitable versions of autoconf.

> There were also numerous warnings when compiling GCC from GCC 2.96 and XGCC
> itself. Is this okay, or is the hope to someday have a smooth, clean build?

You should make sure your changes don't add any new warnings.

> +    warning("return value from function ignored");

Space before open parenthesis.

> +@item -Wunused-returns @r{(C only)}
> +@opindex Wunused-returns
> +Warn whenever the result of a non-void function is implicitly cast away.
> +It is not included in the above, because it requires @code{void} casting
> +returns of all sorts of regular functions, like @code{close} and
> +@code{printf}, whose return value you usually don't care about. This makes
> +GCC more like Lint.

The position you've put this in the file implies it's included in -Wall,
since -Wall below says that all the above -W options are included.  It
should go somewhere below -W in the file.

Please also update trouble.texi so that it says, this is why we don't warn
by default but we have this option if you do want these warnings.  
(Similar to how below it says to use -pedantic-errors to make constraint
violations into errors rather than warnings.)

-- 
Joseph S. Myers
jsm28@cam.ac.uk