[moving this thread to cfe-commits] On Tue, Mar 13, 2012 at 3:04 PM, Lubos Lunak <[email protected]> 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 > [email protected] > > _______________________________________________ > cfe-dev mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
