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

[David Blaikie]

On Tue, Mar 13, 2012 at 3:24 PM, David Blaikie <dblaikie at gmail.com> wrote:
> [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?

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
>>  l.lunak at suse.cz
>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>