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

[David Blaikie]

On Wed, Mar 14, 2012 at 11:54 AM, Lubos Lunak <l.lunak at suse.cz> wrote:
> On Wednesday 14 of March 2012, David Blaikie wrote:
>> On Tue, Mar 13, 2012 at 10:53 PM, Lubos Lunak <l.lunak at suse.cz> wrote:
>> >  Updated patch attached.
>>
>> Great - I have two optional suggestions
>>
>> * You can remove the stddef.h include from test/SemaCXX/conversion.cpp
>
>  Done, attached.

Thanks. Though based on offline feedback from Chandler, I moved the
test cases back to conversion.cpp rather than the separate test file
you'd added - for the sake of test perf (each test file has a
relatively high fixed cost). The only value it was adding was to test
that -Wnull-conversion was on by default (which, granted, was a bug
you had initially) & there's limited value in that.

>> * 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)
>
>  I've left this one as it is. I don't expect the header to break things and
> NULL is what people use in real programs, so in case it actually breaks, it's
> probably better to trigger it here too.

Yep, fair enough - I was on the fence about this.

>> 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.
>
>  I have no idea, obviously. I only followed usage of another conversion
> option. All I can say is that when removing this change passing
> -Wno-conversion-null still disables the warning.

I pinged Chad Rosier (who seemed to be responsible for this list based
on revision history) off list & he confirmed what Chandler told
me/speculated this list was for: some llvm-gcc fallback behavior built
into the clang driver for use on darwin. Your change was correct.

I've committed this, with modifications, as r152745.

* kept the tests in the same file, rather than a separate one
* provided the warning under the name "null-conversion" for
consistency with other *-conversion flags we already have (but
provided a "conversion-null" alias for compatibility with GCC)
* rephrased the bug/comment for "long l = NULL;" so it'll be easily
found when someone fixes that issue
* minor bits & pieces

Thanks again,
- David