[libcxx] r333325 - Add nonnull; use it for atomics

[JF Bastien via cfe-commits]

Hi Marshall,

I’ve been thinking about this, and I propose we proceed the following way:

Add a new warning to clang, -Wnonnull-attribute (modulo name bikeshed), which is part of the nonnull group and on by default. That warning specifically controls warnings generated by usage of the nonnull attribute in code.
Wait a week or two so out-of-sync clang / libc++ codebases can catch up.
Un-revert r333351, adding the nonnull attributes to non-member atomic functions.

That should address the concern you raised because library users can now silence the warning. In this case I think it’s fine to have the warning on by default because:

It’s hard to trigger, you need to directly pass null to the functions, copying a null into the function won’t warn.
The warning has zero false-positives. The code will definitely segfault when you call it.

What do you think?


Thanks,

JF


> On May 26, 2018, at 12:50 PM, JF Bastien via cfe-commits <cfe-commits at lists.llvm.org> wrote:
> 
> 
> 
>> On May 26, 2018, at 12:36 PM, Marshall Clow <mclow.lists at gmail.com <mailto:mclow.lists at gmail.com>> wrote:
>> 
>> 
>> 
>> On Fri, May 25, 2018 at 4:43 PM, JF Bastien via cfe-commits <cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>> wrote:
>> Author: jfb
>> Date: Fri May 25 16:43:53 2018
>> New Revision: 333325
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=333325&view=rev <http://llvm.org/viewvc/llvm-project?rev=333325&view=rev>
>> Log:
>> Add nonnull; use it for atomics
>> 
>> 
>> JF - please revert this patch.
> 
> r333351
> 
>> Let's have a discussion about how to implement this so that it is more friendly to people with installed code bases.
>> [ We've had *extremely* loud responses to unilaterally adding warnings - especially ones that can't be easily disabled - to the libc++ code base in the past. ] 
> 
> Ha! Interesting point which I hadn’t considered. I guess the clang warning can be controlled, but this one cannot? Agreed that’s suboptimal and we should figure out a way to make them controllable.
> 
> 
>> Also, please include both myself and EricWF on all libc++ reviews.
> 
> Sorry, will do. I assumed you’d all have Herald auto-add if you wanted to be on a patch :-)
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180529/6022afa4/attachment.html>