[cfe-commits] [cfe-dev] [PATCH] -Wconversion-null

[David Blaikie]

[moving this thread to cfe-commits]

On Tue, Mar 13, 2012 at 3:04 PM, Lubos Lunak <l.lunak at suse.cz> wrote:
>
>  Hello,
>
>  the attached patch adds option -Wconversion-null . It is pretty much the same
> like http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35669 .
>
>  The purpose of the option can be seen in this testcase:
>
> #include <stdlib.h>
> void foo( int a )
>    {
>    }
> int main()
>    {
>    foo( NULL );
>    int a = NULL;
>    int b = 1;
>    short c = b + 1;
>    (void)a;
>    (void)c;
>    return 0;
>    }
>
> $ clang++ -Wconversion null.cpp -c -fno-caret-diagnostics
> null.cpp:9:10: warning: implicit conversion of NULL constant to integer
> [-Wconversion]
> null.cpp:10:13: warning: implicit conversion of NULL constant to integer
> [-Wconversion]
> null.cpp:12:15: warning: implicit conversion loses integer precision: 'int'
> to 'short' [-Wconversion]

OK - so it's not so much that you're adding a new warning, as moving
some existing warning under a different/more specific (& consistent
with GCC) flag. Makes sense.

>  There are two obviously incorrect uses of NULL in this testcase. This is
> currently warned about only with -Wconversion, which however also triggers
> other conversion warnings that are not obviously incorrect. There are
> probably no realistic usage scenarios where a conversion from NULL to integer
> would be intended, but e.g. short<->int conversions may be wanted for example
> for space saving reasons and casts that would silence those warnings may not
> be deemed worth the decreased readability. In short, the benefits
> of -Wconversion may be questionable depending on the codebase, but incorrect
> usage of NULL should not (and it may not be as obvious as in this testcase).
>
>  This can be solved by introduction of -Wconversion-null, which only warns
> about most probably incorrect usage of NULL, and is enabled by -Wconversion
> or -Wall.
>
>  The attached patch should implement the change. This is my first clang
> contribution, so I don't know if there is something more necessary. I also
> couldn't find if/where options are documented.

There's a bit of documentation on the website (which is in the www svn
project) but it's nothing to really worry about at this point - it
only documents a handful of warning flags.

As for the patch itself: Why are you removing the test cases?
If you're going to provide a tblgen name for the ConversionNull group,
perhaps you could define it above the Conversion group & use the
ConversionNull name (rather than using the "conversion-null" string
separately/twice) in Conversion's definition.

It's probably OK to remove the "DefaultIgnore" flag for this patch
too, again to be consistent with GCC (which has this warning on by
default).

One caveat is that this warning is currently a little bit broken in
Clang. If you initialize an integer of the same size as a pointer, the
warning will not be provided. i.e. only one warning is produced from:

int i = __null;
long l = __null;

(__null is the special null value that GNU's stdlib defines NULL to be
& how it detects this case) depending on whether you have a 32 bit or
64 bit pointer (assuming you have one of those and that int is 32 bits
and long is 64). That's something to fix at some point...

- David
>
> --
>  Lubos Lunak
>  l.lunak at suse.cz
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>