Re: PATCH: warn on memset with constant length 0

["Manuel LÃpez-IbÃÃez" <lopezibanez at gmail dot com>]

On 15 Dec 2006 08:39:48 -0500, Assar <assar@kth.se> wrote:

When creating the patch, you should use the -p option of diff, please.
It helps a lot when reviewing the patch.

I am not a maintainer and I cannot approve your patch but I hope you
find the following comments useful.

> Index: gcc/testsuite/gcc.dg/memset-constant0-3.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/memset-constant0-3.c   (revision 0)
> +++ gcc/testsuite/gcc.dg/memset-constant0-3.c   (revision 0)
> @@ -0,0 +1,15 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Wextra -O3" } */
> +

Why use Wextra here? Is not Wmemset enabled by Wall ? Nevertheless, I
think that the appropriate is to use Wmemset directly or even -Wextra
-Wall -Wmemset all together.

Index: gcc/c.opt
> +Wmemset
> +C objC C++ ObjC++
> +Warn about possible bad uses of \"memset\"
> +

Why don't you use this instead ?

Wmemset
C ObjC C++ ObjC++ Var(warn_memset) Init(-1)
Warn about possible bad uses of \"memset\"

With the above changes, I think that the following are unnecessary:

> Index: gcc/c-opts.c
> ===================================================================
> --- gcc/c-opts.c        (revision 119393)
> +++ gcc/c-opts.c        (working copy)
> @@ -459,6 +460,10 @@
>        set_Wformat (atoi (arg));
>        break;
>
> +    case OPT_Wmemset:
> +      warn_memset = value;
> +      break;
> +

Not needed since it is already defined in gcc/c.opt .

> Index: gcc/c-common.c
> ===================================================================
> --- gcc/c-common.c      (revision 119393)
> +++ gcc/c-common.c      (working copy)
> @@ -274,6 +274,10 @@
>
>  int warn_format;
>
> +/* Warn about likely bad uses of memset. */
> +
> +int warn_memset;
> +

Again, I think this is redundant since the variable is defined in gcc/c.opt

>  /* Warn about using __null (as NULL in C++) as sentinel.  For code compiled
>     with GCC this doesn't matter as __null is guaranteed to have the right
>     size.  */
> @@ -5827,6 +5831,30 @@
>    return NULL_TREE;
>  }
>
> +/* Check arguments passed to memset.  */
> +static void
> +check_function_memset (tree params)
> +{
> +  tree param;
> +  int param_num;
> +
> +  for (param = params, param_num = 1;
> +       param;
> +       param_num++, param = TREE_CHAIN (param))
> +    if (param_num == 3 && integer_zerop (TREE_VALUE (param)))
> +      warning (OPT_Wextra, "memset called with a constant 0 length");
> +}
> +

You have to use OPT_Wmemset instead of OPT_Wextra.

Apart from that, is there no way to access the third argument
directly? That "for" gives me the creeps.


> Index: gcc/c-common.h
> ===================================================================
> --- gcc/c-common.h      (revision 119393)
> +++ gcc/c-common.h      (working copy)
> @@ -417,7 +417,11 @@
>
>  extern int warn_format;
>
> +/* Warn about likely bad uses of memset. */
>
> +extern int warn_memset;
> +
> +
>  /* C/ObjC language option variables.  */

I think this is also redundant.


Cheers,

Manuel.