On Tue, Mar 13, 2012 at 10:53 PM, Lubos Lunak <[email protected]> wrote: > On Tuesday 13 of March 2012, David Blaikie wrote: >> On Tue, Mar 13, 2012 at 3:24 PM, David Blaikie <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
