On Tue, Mar 13, 2012 at 3:24 PM, David Blaikie <[email protected]> wrote: > [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?
Ah, sorry - I see you moved the test cases. This is probably reasonable. > 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. And reuse the ConversionNull named group in DiagnosticSemaKinds.td too. The "conversion-null" string should be written once rather than three times. > 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). Hmm, weird - I'm looking at the DiagnosticSemaKinds.td & I'm not sure why this warning is actually on by default & your test case is passing. (assuming it is) > 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
