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@lists.llvm.org> wrote: > > > >> On May 26, 2018, at 12:36 PM, Marshall Clow <mclow.li...@gmail.com >> <mailto:mclow.li...@gmail.com>> wrote: >> >> >> >> On Fri, May 25, 2018 at 4:43 PM, JF Bastien via cfe-commits >> <cfe-commits@lists.llvm.org <mailto:cfe-commits@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@lists.llvm.org <mailto:cfe-commits@lists.llvm.org> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
