[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [cfe-commits] [PATCH] -Wconversion-null
From:       David Blaikie <dblaikie () gmail ! com>
Date:       2012-03-14 22:38:03
Message-ID: CAENS6EuQa5PMMC5O6t-L1uOUDA_GG0hE6t2j7thM54Nq9AJi9Q () mail ! gmail ! com
[Download RAW message or body]

On Wed, Mar 14, 2012 at 11:54 AM, Lubos Lunak <l.lunak@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@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

_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]