[cfe-commits] [PATCH] -Wconversion-null
dblaikie at gmail.com
Wed Mar 14 15:38:03 PDT 2012
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
> 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
More information about the cfe-commits