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

[David Blaikie]

On Tue, Mar 13, 2012 at 10:53 PM, Lubos Lunak <l.lunak at suse.cz> wrote:
> On Tuesday 13 of March 2012, David Blaikie wrote:
>> On Tue, Mar 13, 2012 at 3:24 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> > 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.
>
>  Updated patch attached.

Great - I have two optional suggestions

* You can remove the stddef.h include from test/SemaCXX/conversion.cpp
* You could change the NULL usage in conversion-gnunull.cpp to __null
& drop the header to simplify/isolate the test case a bit further.
(this is, perhaps, debatable - as it doesn't quite test the "real" use
case)

and one question I can't answer that blocks me from signing
off/checking this in for you:

Is the change in lib/Driver/Tools.cpp necessary/correct? I don't fully
understand what this list is for. Chandler helped me/speculated that
this is a list of all Clang options that don't exist in GCC 4.2 - but
if that's the case it's already broken (even by changes I've made -
such as adding the -Wcovered-switch-default flag which I never added
to this list). So I'm wondering where this list is tested/validated.
Hopefully an Apple Clang developer can chime in here & explain this.

>> > 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)
>
>  It is actually not. I simply copy&pasted it to a separate file, and I
> ran 'make check', which ran LLVM's checks, and 'make unittest' in clang/,
> which didn't run the tests in tests/ either. The NULL tests pass with the
> updated patch when I run 'make' in clang/tests/.

Great. (by the way you can run "make check-all" I think (or check-lit?
I don't remember as I've been using the CMake-based build recently)

>
>> > 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...
>
>  I see. I'll leave that as a separate issue.

Yep - and I see you set the target type in your new test file, which
is good/necessary due to this bug.

- David