On Wed, Mar 14, 2012 at 11:54 AM, Lubos Lunak <[email protected]> wrote: > On Wednesday 14 of March 2012, David Blaikie wrote: >> On Tue, Mar 13, 2012 at 10:53 PM, Lubos Lunak <[email protected]> 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 [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
